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