You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@seata.apache.org by "Sumit6307 (via GitHub)" <gi...@apache.org> on 2026/01/30 07:34:40 UTC
[PR] fix: unregister resource when closing DataSourceProxy [incubator-seata]
Sumit6307 opened a new pull request, #7981:
URL: https://github.com/apache/incubator-seata/pull/7981
Implemented the TODO item in DataSourceProxy.close() to ensure that resources are properly unregistered from the
DefaultResourceManager
when the proxy is closed. This prevents potential resource leaks by ensuring DefaultResourceManager.get().unregisterResource(this) is called.
Ⅱ. Does this pull request fix one issue?
Fixes missing resource cleanup in
DataSourceProxy
.
Ⅲ. Why don't you add test cases (unit test/integration test)?
I have updated the existing unit test
DataSourceProxyTest
. Specifically, I modified
testCloseRemovesResource
to verify that
unregisterResource
is called when the proxy is closed.
Ⅳ. Describe how to verify it
Run the unit tests for the datasource module: mvn -pl rm-datasource -Dtest=DataSourceProxyTest test
Ⅴ. Special notes for reviews
This change simply addresses an existing TODO comment to ensure cleaner resource lifecycle management.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "Sumit6307 (via GitHub)" <gi...@apache.org>.
Sumit6307 commented on PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#issuecomment-4007432540
@funky-eyes Review this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "WangzJi (via GitHub)" <gi...@apache.org>.
WangzJi commented on PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#issuecomment-4242696752
> > Hi @Sumit6307 , thanks for working on this! The overall direction is right — implementing UnregisterRMRequest to clean up resources on the server side. I have a few concerns after reviewing:
> >
> > 1. Missing Serialization Codec (Critical)
> > The PR adds `TYPE_UNREG_RM = 105` and `TYPE_UNREG_RM_RESULT = 106` in MessageType, but does not add the corresponding Codec classes or register them in MessageCodecFactory. In Seata's serialization framework, every message type requires an explicit Codec registered in MessageCodecFactory.getMessageCodec() — there is no auto-discovery. Without this, sending an UnRegisterRMRequest will hit the default branch and throw: `IllegalArgumentException("not support typeCode, 105")`
> > You need to add:
> >
> > * UnregisterRMRequestCodec (can extend AbstractIdentifyRequestCodec, similar to RegisterRMRequestCodec)
> > * UnregisterRMResponseCodec (similar to RegisterRMResponseCodec)
> > * Register both in MessageCodecFactory.getMessageCodec() and MessageCodecFactory.getMessage()
> > 2. Prerequisite: Server Version Caching
> > [Caching RPC Context and Server Version #7593](https://github.com/apache/incubator-seata/issues/7593) is a prerequisite for [Implement Client-Side Resource Unregistration #7594](https://github.com/apache/incubator-seata/issues/7594) — the server version needs to be cached from RegisterRMResponse first, so it can be used for version-aware unregistration. The current approach uses getChannelVersion() inline, but that retrieves the client-side version stored during channel registration, not the actual server version from the response. The server version should be cached when RegisterRMResponse is received (in onRegisterMsgSuccess). This PR should be based on [Caching RPC Context and Server Version #7593](https://github.com/apache/incubator-seata/issues/7593) being resolved first.
> > 3. Missing destroy() Unregistration
> > The related issue [Implement Client-Side Resource Unregistration #7594](https://github.com/apache/incubator-seata/issues/7594) explicitly requires a destroyAll interface — when the client node goes offline, it should proactively notify the server to unregister all resources. Currently RmNettyRemotingClient.destroy() is not modified, so client shutdown won't trigger any unregistration.
> >
> > FYI, I've submitted PR #8020 which covers both with complete serialization codec support, version caching, destroy() unregistration, and unit tests. You're welcome to take a look!
>
> Done. Please Review @WangzJi @funky-eyes
Hi @Sumit6307, thanks for the update.
Some parts of this PR are already covered by #8020, which has been merged. But the DataSourceProxy.close() cleanup part is still worth moving forward.
Could you rebase it on the latest 2.x, fix the conflicts？Then we can continue reviewing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "Sumit6307 (via GitHub)" <gi...@apache.org>.
Sumit6307 commented on code in PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#discussion_r3118673138
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/DataSourceProxyTest.java:
##########
@@ -237,7 +237,33 @@ public void testCloseRemovesResource() throws Exception {
proxy.close();
+ verify(drm).unregisterResource(proxy);
tmcfStatic.verify(() -> TableMetaCacheFactory.shutdown(proxy.getResourceId()));
+ verify(drm).unregisterResource(proxy);
Review Comment:
> nit: duplicate `verify(drm).unregisterResource(proxy)` — this line can be removed.
@WangzJi
Done. Removed the duplicate `verify` line as suggested.
Thanks for pointing that out! Let me know if everything else looks good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "Sumit6307 (via GitHub)" <gi...@apache.org>.
Sumit6307 commented on PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#issuecomment-4126355427
> Hi @Sumit6307 , thanks for working on this! The overall direction is right — implementing UnregisterRMRequest to clean up resources on the server side. I have a few concerns after reviewing:
>
> 1. Missing Serialization Codec (Critical)
> The PR adds `TYPE_UNREG_RM = 105` and `TYPE_UNREG_RM_RESULT = 106` in MessageType, but does not add the corresponding Codec classes or register them in MessageCodecFactory. In Seata's serialization framework, every message type requires an explicit Codec registered in MessageCodecFactory.getMessageCodec() — there is no auto-discovery. Without this, sending an UnRegisterRMRequest will hit the default branch and throw: `IllegalArgumentException("not support typeCode, 105")`
> You need to add:
> - UnregisterRMRequestCodec (can extend AbstractIdentifyRequestCodec, similar to RegisterRMRequestCodec)
> - UnregisterRMResponseCodec (similar to RegisterRMResponseCodec)
> - Register both in MessageCodecFactory.getMessageCodec() and MessageCodecFactory.getMessage()
> 2. Prerequisite: Server Version Caching
> [Caching RPC Context and Server Version #7593](https://github.com/apache/incubator-seata/issues/7593) is a prerequisite for [Implement Client-Side Resource Unregistration #7594](https://github.com/apache/incubator-seata/issues/7594) — the server version needs to be cached from RegisterRMResponse first, so it can be used for version-aware unregistration. The current approach uses getChannelVersion() inline, but that retrieves the client-side version stored during channel registration, not the actual server version from the response. The server version should be cached when RegisterRMResponse is received (in onRegisterMsgSuccess). This PR should be based on [Caching RPC Context and Server Version #7593](https://github.com/apache/incubator-seata/issues/7593) being resolved first.
> 3. Missing destroy() Unregistration
> The related issue [Implement Client-Side Resource Unregistration #7594](https://github.com/apache/incubator-seata/issues/7594) explicitly requires a destroyAll interface — when the client node goes offline, it should proactively notify the server to unregister all resources. Currently RmNettyRemotingClient.destroy() is not modified, so client shutdown won't trigger any unregistration.
>
> FYI, I've submitted PR #8020 which covers both with complete serialization codec support, version caching, destroy() unregistration, and unit tests. You're welcome to take a look!
Done. Please Review @WangzJi @funky-eyes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "Sumit6307 (via GitHub)" <gi...@apache.org>.
Sumit6307 commented on PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#issuecomment-4286004615
> Hi @Sumit6307, two issues after testing locally:
>
> 1. `super.unregisterResource(...)` in `DataSourceManager` throws `NotSupportYetException` (parent still throws). `close()` fails on
> every call, `TableMetaCacheFactory.shutdown` never runs. Drop the super call, or delegate to
> `RmNettyRemotingClient.getInstance().unregisterResource(group, resourceId)` from #8020.
> 2. The test mocks `DefaultResourceManager.get()`, so the modified `DataSourceManager.unregisterResource()` never executes — that's
> why CI passes. Please add a test without mocking `DefaultResourceManager` that asserts `close()` doesn't throw and the cache entry
> is removed.
>
> Also suggest try/finally in `close()` so `TableMetaCacheFactory.shutdown` runs even if unregister fails.
Hi @WangzJi and @funky-eyes,
I've updated the PR to address these specific issues:
1. **Dropped `super.unregisterResource`**:
In `DataSourceManager`, I removed the `super.unregisterResource(dataSourceProxy)` call so it no longer throws `NotSupportYetException`. Instead, it now just removes the resource from the `dataSourceCache` and delegates directly to `RmNettyRemotingClient.getInstance().unregisterResource()`.
2. **Added `try/finally` block in `DataSourceProxy.close()`**:
I wrapped the unregistration inside a `try/finally` block. Now, `TableMetaCacheFactory.shutdown(resourceId)` is guaranteed to run even if the `unregisterResource` call fails or throws an exception.
3. **Added Real Integration Test**:
I added a new test `testCloseRemovesResourceFromManager` in `DataSourceProxyTest` that creates a real `DruidDataSource` and `DataSourceProxy` without mocking `DefaultResourceManager.get()`. It asserts that the proxy is properly removed from the system's active managed resources cache without throwing any exceptions during `close()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "Sumit6307 (via GitHub)" <gi...@apache.org>.
Sumit6307 commented on PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#issuecomment-3866294861
> Implemented the TODO item in DataSourceProxy.close() to ensure that resources are properly unregistered from the DefaultResourceManager when the proxy is closed. This prevents potential resource leaks by ensuring DefaultResourceManager.get().unregisterResource(this) is called.
>
> Ⅱ. Does this pull request fix one issue? Fixes missing resource cleanup in DataSourceProxy .
>
> Ⅲ. Why don't you add test cases (unit test/integration test)? I have updated the existing unit test DataSourceProxyTest . Specifically, I modified testCloseRemovesResource to verify that unregisterResource is called when the proxy is closed.
>
> Ⅳ. Describe how to verify it Run the unit tests for the datasource module: mvn -pl rm-datasource -Dtest=DataSourceProxyTest test
>
> Ⅴ. Special notes for reviews This change simply addresses an existing TODO comment to ensure cleaner resource lifecycle management.
Sir @funky-eyes Please Review this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "Sumit6307 (via GitHub)" <gi...@apache.org>.
Sumit6307 commented on PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#issuecomment-4283709997
> > > Hi @Sumit6307 , thanks for working on this! The overall direction is right — implementing UnregisterRMRequest to clean up resources on the server side. I have a few concerns after reviewing:
> > >
> > > 1. Missing Serialization Codec (Critical)
> > > The PR adds `TYPE_UNREG_RM = 105` and `TYPE_UNREG_RM_RESULT = 106` in MessageType, but does not add the corresponding Codec classes or register them in MessageCodecFactory. In Seata's serialization framework, every message type requires an explicit Codec registered in MessageCodecFactory.getMessageCodec() — there is no auto-discovery. Without this, sending an UnRegisterRMRequest will hit the default branch and throw: `IllegalArgumentException("not support typeCode, 105")`
> > > You need to add:
> > >
> > > * UnregisterRMRequestCodec (can extend AbstractIdentifyRequestCodec, similar to RegisterRMRequestCodec)
> > > * UnregisterRMResponseCodec (similar to RegisterRMResponseCodec)
> > > * Register both in MessageCodecFactory.getMessageCodec() and MessageCodecFactory.getMessage()
> > > 2. Prerequisite: Server Version Caching
> > > [Caching RPC Context and Server Version #7593](https://github.com/apache/incubator-seata/issues/7593) is a prerequisite for [Implement Client-Side Resource Unregistration #7594](https://github.com/apache/incubator-seata/issues/7594) — the server version needs to be cached from RegisterRMResponse first, so it can be used for version-aware unregistration. The current approach uses getChannelVersion() inline, but that retrieves the client-side version stored during channel registration, not the actual server version from the response. The server version should be cached when RegisterRMResponse is received (in onRegisterMsgSuccess). This PR should be based on [Caching RPC Context and Server Version #7593](https://github.com/apache/incubator-seata/issues/7593) being resolved first.
> > > 3. Missing destroy() Unregistration
> > > The related issue [Implement Client-Side Resource Unregistration #7594](https://github.com/apache/incubator-seata/issues/7594) explicitly requires a destroyAll interface — when the client node goes offline, it should proactively notify the server to unregister all resources. Currently RmNettyRemotingClient.destroy() is not modified, so client shutdown won't trigger any unregistration.
> > >
> > > FYI, I've submitted PR #8020 which covers both with complete serialization codec support, version caching, destroy() unregistration, and unit tests. You're welcome to take a look!
> >
> >
> > Done. Please Review @WangzJi @funky-eyes
>
> Hi @Sumit6307, thanks for the update.
>
> Some parts of this PR are already covered by #8020, which has been merged. But the DataSourceProxy.close() cleanup part is still worth moving forward.
>
> Could you rebase it on the latest 2.x, fix the conflicts？Then we can continue reviewing it.
Here is a professional and clear reply you can copy and paste into the GitHub PR conversation to update the maintainers:
Hi @WangzJi and @funky-eyes,
Thanks for the guidance! I've updated the PR as requested:
- **Rebased on the latest `2.x` branch**, which resolved the merge conflicts.
- **Removed the duplicate serialization and UnregisterRM protocol changes**, since those have already been successfully merged in #8020.
- **Retained the specific `DataSourceProxy.close()` and `DataSourceManager.unregisterResource()` cleanup logic**, along with their corresponding unit tests to prevent the original resource leak issue.
- Verified that the `Spotless` code format checks are passing cleanly.
The PR is now clean and isolates only the `DataSourceProxy` resource unregistration fix. Please let me know if anything else is needed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "WangzJi (via GitHub)" <gi...@apache.org>.
WangzJi merged PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "Sumit6307 (via GitHub)" <gi...@apache.org>.
Sumit6307 commented on PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#issuecomment-3852848957
@funky-eyes Sir Please Check and merge this PR #7981
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "Sumit6307 (via GitHub)" <gi...@apache.org>.
Sumit6307 commented on PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#issuecomment-4129086101
Hi @funky-eyes ! Just a quick heads-up outside of the PR review: I am currently submitting my GSoC 2026 proposal for Apache Seata, titled **"Enhance Seata Go SDK Multi Registry Support and Seata CTL Diagnostic Tool"**.
I saw FinnTew is listed as a potential mentor. Since you've reviewed most of my recent merged and open PRs, I just wanted to ask if you are also going to be mentoring any GSoC projects this year?
Thanks again for all your reviews on my code so far!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "WangzJi (via GitHub)" <gi...@apache.org>.
WangzJi commented on code in PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#discussion_r3118294375
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/DataSourceProxyTest.java:
##########
@@ -237,7 +237,33 @@ public void testCloseRemovesResource() throws Exception {
proxy.close();
+ verify(drm).unregisterResource(proxy);
tmcfStatic.verify(() -> TableMetaCacheFactory.shutdown(proxy.getResourceId()));
+ verify(drm).unregisterResource(proxy);
Review Comment:
nit: duplicate `verify(drm).unregisterResource(proxy)` — this line can be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#issuecomment-3822506300
## [Codecov](https://app.codecov.io/gh/apache/incubator-seata/pull/7981?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 71.78%. Comparing base ([`9e6a843`](https://app.codecov.io/gh/apache/incubator-seata/commit/9e6a84364cf078b363c241d5f62b9b0ad378c6d2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4c6b24c`](https://app.codecov.io/gh/apache/incubator-seata/commit/4c6b24c62291eae5c2fc461df281659643750866?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 2.x #7981 +/- ##
============================================
+ Coverage 71.74% 71.78% +0.03%
Complexity 872 872
============================================
Files 1300 1300
Lines 49727 49728 +1
Branches 5908 5908
============================================
+ Hits 35678 35698 +20
+ Misses 11114 11094 -20
- Partials 2935 2936 +1
```
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-seata/pull/7981?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...rg/apache/seata/rm/datasource/DataSourceProxy.java](https://app.codecov.io/gh/apache/incubator-seata/pull/7981?src=pr&el=tree&filepath=rm-datasource%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fseata%2Frm%2Fdatasource%2FDataSourceProxy.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cm0tZGF0YXNvdXJjZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2VhdGEvcm0vZGF0YXNvdXJjZS9EYXRhU291cmNlUHJveHkuamF2YQ==) | `64.76% <100.00%> (+0.33%)` | :arrow_up: |
... and [9 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-seata/pull/7981/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
[](https://app.codecov.io/gh/apache/incubator-seata/pull/7981?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "funky-eyes (via GitHub)" <gi...@apache.org>.
funky-eyes commented on code in PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#discussion_r2893223218
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/DataSourceProxy.java:
##########
@@ -294,7 +294,7 @@ public String getUserName() {
}
public void close() throws Exception {
- // TODO: Need to unregister resource from DefaultResourceManager
+ DefaultResourceManager.get().unregisterResource(this);
Review Comment:
This operation must send a resource-unload request to the server; if the resource is only unloaded locally, the server will be unaware and may still dispatch phase-two execution tasks to that node. It also needs to be version-aware: if the client is a newer version than the server, the server might not support the unload-resource request at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "funky-eyes (via GitHub)" <gi...@apache.org>.
funky-eyes commented on code in PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#discussion_r2945594924
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/DataSourceManager.java:
##########
@@ -93,7 +93,9 @@ public void registerResource(Resource resource) {
@Override
public void unregisterResource(Resource resource) {
- throw new NotSupportYetException("unregister a resource");
+ DataSourceProxy dataSourceProxy = (DataSourceProxy) resource;
+ dataSourceCache.remove(dataSourceProxy.getResourceId());
+ super.unregisterResource(dataSourceProxy);
Review Comment:
```suggestion
super.unregisterResource(dataSourceProxy);
DataSourceProxy dataSourceProxy = (DataSourceProxy) resource;
dataSourceCache.remove(dataSourceProxy.getResourceId());
```
I believe we should first deregister the resource and then remove it locally to avoid a situation where the server continues sending operations for a resource that has already been deleted on the client. Additionally, I’m wondering if it might be necessary to introduce a delay (e.g., sleep for a while) to account for potential network issues, ensuring that any requests still needing the resource have time to complete. This is because the deregistration request sent to the server and the server’s subsequent instructions sent to the client are processed in parallel. Even though the resource has been deregistered on the server, requests already in progress before the deregistration might still be sent.
##########
core/src/main/java/org/apache/seata/core/rpc/netty/RmNettyRemotingClient.java:
##########
@@ -275,6 +277,46 @@ public void registerResource(String resourceGroupId, String resourceId) {
}
}
+ /**
+ * Unregister resource.
+ *
+ * @param resourceGroupId the resource group id
+ * @param resourceId the resource id
+ */
+ public void unRegisterResource(String resourceGroupId, String resourceId) {
+ if (StringUtils.isBlank(transactionServiceGroup) || StringUtils.isBlank(resourceId)) {
+ return;
+ }
+
+ if (getClientChannelManager().getChannels().isEmpty()) {
+ return;
+ }
+
+ synchronized (getClientChannelManager().getChannels()) {
+ for (Map.Entry<String, Channel> entry : getClientChannelManager().getChannels().entrySet()) {
+ Channel rmChannel = entry.getValue();
+ String serverVersion = getClientChannelManager().getChannelVersion(rmChannel);
+ // Threshold for version-aware unregistration: version >= 2.1.0
Review Comment:
Why is it version 2.1.0 or above, rather than 2.7.0 or above?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: unregister resource when closing DataSourceProxy [incubator-seata]
Posted by "Sumit6307 (via GitHub)" <gi...@apache.org>.
Sumit6307 commented on PR #7981:
URL: https://github.com/apache/incubator-seata/pull/7981#issuecomment-4283723642
> > > Hi @Sumit6307 , thanks for working on this! The overall direction is right — implementing UnregisterRMRequest to clean up resources on the server side. I have a few concerns after reviewing:
> > >
> > > 1. Missing Serialization Codec (Critical)
> > > The PR adds `TYPE_UNREG_RM = 105` and `TYPE_UNREG_RM_RESULT = 106` in MessageType, but does not add the corresponding Codec classes or register them in MessageCodecFactory. In Seata's serialization framework, every message type requires an explicit Codec registered in MessageCodecFactory.getMessageCodec() — there is no auto-discovery. Without this, sending an UnRegisterRMRequest will hit the default branch and throw: `IllegalArgumentException("not support typeCode, 105")`
> > > You need to add:
> > >
> > > * UnregisterRMRequestCodec (can extend AbstractIdentifyRequestCodec, similar to RegisterRMRequestCodec)
> > > * UnregisterRMResponseCodec (similar to RegisterRMResponseCodec)
> > > * Register both in MessageCodecFactory.getMessageCodec() and MessageCodecFactory.getMessage()
> > > 2. Prerequisite: Server Version Caching
> > > [Caching RPC Context and Server Version #7593](https://github.com/apache/incubator-seata/issues/7593) is a prerequisite for [Implement Client-Side Resource Unregistration #7594](https://github.com/apache/incubator-seata/issues/7594) — the server version needs to be cached from RegisterRMResponse first, so it can be used for version-aware unregistration. The current approach uses getChannelVersion() inline, but that retrieves the client-side version stored during channel registration, not the actual server version from the response. The server version should be cached when RegisterRMResponse is received (in onRegisterMsgSuccess). This PR should be based on [Caching RPC Context and Server Version #7593](https://github.com/apache/incubator-seata/issues/7593) being resolved first.
> > > 3. Missing destroy() Unregistration
> > > The related issue [Implement Client-Side Resource Unregistration #7594](https://github.com/apache/incubator-seata/issues/7594) explicitly requires a destroyAll interface — when the client node goes offline, it should proactively notify the server to unregister all resources. Currently RmNettyRemotingClient.destroy() is not modified, so client shutdown won't trigger any unregistration.
> > >
> > > FYI, I've submitted PR #8020 which covers both with complete serialization codec support, version caching, destroy() unregistration, and unit tests. You're welcome to take a look!
> >
> >
> > Done. Please Review @WangzJi @funky-eyes
>
> Hi @Sumit6307, thanks for the update.
>
> Some parts of this PR are already covered by #8020, which has been merged. But the DataSourceProxy.close() cleanup part is still worth moving forward.
>
> Could you rebase it on the latest 2.x, fix the conflicts？Then we can continue reviewing it.
Hi @WangzJi and @funky-eyes,
Thanks for the guidance! I've updated the PR as requested:
- **Rebased on the latest `2.x` branch**, which resolved the merge conflicts.
- **Removed the duplicate serialization and UnregisterRM protocol changes**, since those have already been successfully merged in #8020.
- **Retained the specific `DataSourceProxy.close()` and `DataSourceManager.unregisterResource()` cleanup logic**, along with their corresponding unit tests to prevent the original resource leak issue.
- Verified that the `Spotless` code format checks are passing cleanly.
The PR is now clean and isolates only the `DataSourceProxy` resource unregistration fix. Please let me know if anything else is needed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org