fuyou001 commented on PR #10689: URL: https://github.com/apache/rocketmq/pull/10689#issuecomment-5128558419
The validation direction looks useful, but I think a few compatibility and resource-safety cases should be addressed before merging. 1. `TopicRouteWrapper.getMasterAddr` changes its public return type from `String` to `Optional<String>`. This is source- and binary-incompatible for external extensions or code compiled against the proxy module. Please consider retaining the existing signature and adding a separate safe lookup method, unless this type is explicitly guaranteed not to be an extension API. 2. The queue count is only checked for integer syntax. A negative value silently produces no queues, while a very large positive value can allocate a large number of `AddressableMessageQueue` objects during route refresh. Please require a positive count and enforce a reasonable upper bound based on RocketMQ queue limits. 3. Partially accepting a damaged ordered-topic configuration changes the ordered queue set. Please confirm that partial acceptance is the desired consistency policy; otherwise, rejecting the whole new configuration and retaining the last valid route would avoid different nodes constructing different queue views during mixed-version operation. Please add regression coverage for zero, negative, `Integer.MAX_VALUE`, overflow, all-items-invalid, and missing-master cases; route refresh/cache recovery with a previously valid route; and mixed old/new proxy behavior. Please also assert how the new `ProxyException` from `ClusterTopicRouteService.getBrokerAddr` is mapped and propagated through `ConsumerProcessor`, `ClusterMessageService`, `buildAddressableMessageQueue`, and the background fault-strategy callback, including exceptional future completion. -- 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]
