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: [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]
