zhishengzhang commented on PR #10287: URL: https://github.com/apache/rocketmq/pull/10287#issuecomment-4426780517
> > @yx9o @zhishengzhang > > Sorry for the late reply. > > Just to confirm, this PR and #10283 are both trying to address #10284, but the approaches look different. #10283 changes the generic RemotingSerializable decode path, while this PR narrows the fix to the ConsumerOffset response shape. > > If we keep this approach, could we add a regression test covering the mixed offsetTable + pullOffsetTable response returned by GET_ALL_CONSUMER_OFFSET? Thanks @ChineseTony for the fix and @yx9o for the review. I'd like to share some findings from my investigation in #10283. I had already provided detailed analysis and verification there, but unfortunately those points were not addressed before this separate PR was created. 1. The try-catch fallback in RemotingSerializable is always necessary When upgrading RocketMQ from 4.9.x → 5.4.x → 5.5.x, the Slave node throws com.alibaba.fastjson2.JSONException: not support unquoted name or illegal number during consumerOffset synchronization. Regardless of whether pullOffsetTable and groupTopicMap are added to ConsumerOffsetSerializeWrapper, the try-catch fallback in RemotingSerializable is always necessary — because cross-version serialized data may contain undeclared fields that we cannot predict today, and a single deserialization failure should not crash the entire slave sync process. 2. Comprehensive check of all SerializeWrapper classes I've completed checking all SerializeWrapper classes in the codebase: ConsumerOffsetSerializeWrapper — has unquoted numeric keys and version mismatch (this PR fixes it) TopicConfigAndMappingSerializeWrapper, TopicConfigSerializeWrapper, SubscriptionGroupWrapper OffsetSerializeWrapper, TopicQueueMappingSerializeWrapper, MessageRequestModeSerializeWrapper DelayOffsetSerializeWrapper, KVConfigSerializeWrapper, RocksDBOffsetSerializeWrapper TransactionMetricsSerializeWrapper All other wrappers use String keys and have consistent fields across versions, so no action needed for them. That said, I agree this PR is the right immediate fix for #10284 — it's targeted and low-risk. The RemotingSerializable hardening can be considered as a follow-up improvement. Regarding the regression test @yx9o suggested — I'd be happy to help add one covering the mixed offsetTable + pullOffsetTable response scenario if 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]
