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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
