RockteMQ-AI commented on PR #10717: URL: https://github.com/apache/rocketmq/pull/10717#issuecomment-5140966893
## Review by github-manager-bot ### Summary This PR replaces raw `Settings` protobuf object logging in `ClientActivity` during cross-proxy settings sync. The debug log now outputs a summary (`clientType`, `publishingTopicCount`, `subscriptionCount`) instead of the full `Settings` object. ### Changes Reviewed - `proxy/src/main/java/.../ClientActivity.java` — added `summarizeSettings()` helper, updated log statement - `proxy/src/test/java/.../ClientActivityTest.java` — added test verifying no resource names leak into summary ### Assessment **✅ Correctness** — Logic is correct. Null check and optional field guards are properly implemented. **✅ Performance** — No concerns. Debug-level logging, lightweight summary method. **✅ Tests** — Good coverage. Test verifies both producer and consumer settings summaries exclude resource names, and null input returns "null". **✅ Compatibility** — No public API changes. Package-private static method. ### Suggestion Same observation as #10721 and #10719: the `summarizeSettings()` pattern is now duplicated in 3 classes. A shared utility class would reduce maintenance burden and ensure consistency if the summary format needs to evolve. **Overall: Looks good.** 👍 -- 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]
