RockteMQ-AI commented on PR #10721: URL: https://github.com/apache/rocketmq/pull/10721#issuecomment-5140963076
## Review by github-manager-bot ### Summary This PR replaces raw `Settings` protobuf object logging in `GrpcClientChannel` with a summarized version that only logs `clientType`, `publishingTopicCount`, and `subscriptionCount` — avoiding potential exposure of sensitive topic/resource names in logs. It also replaces full attribute string logging with just the attribute length in the parse error path. ### Changes Reviewed - `proxy/src/main/java/.../GrpcClientChannel.java` — added `summarizeSettings()` and `getAttributeLength()` helpers - `proxy/src/test/java/.../GrpcClientChannelTest.java` — added tests for both helpers ### Assessment **✅ Correctness** — Logic is sound. Null checks are in place. The summary correctly uses `hasPublishing()`/`hasSubscription()` guards before accessing counts. **✅ Performance** — No concerns. The summary method is lightweight and only called in error paths. **✅ Tests** — Good coverage including null input, producer-only, consumer-only, and combined settings scenarios. Tests verify that sensitive resource names do not appear in the summary output. **✅ Compatibility** — No public API changes. Internal helper methods are package-private. ### Suggestion The `summarizeSettings()` logic is duplicated across `GrpcClientChannel`, `GrpcClientSettingsManager`, and `ClientActivity` (see related PRs #10719, #10717). Consider extracting this into a shared utility class (e.g., `SettingsLogHelper`) to follow DRY principle and make future changes easier to maintain. This can be addressed in a follow-up; the current PR is clean on its own. **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]
