RockteMQ-AI commented on PR #10719:
URL: https://github.com/apache/rocketmq/pull/10719#issuecomment-5140966624
## Review by github-manager-bot
### Summary
This PR replaces raw `Settings` protobuf object logging in
`GrpcClientSettingsManager` when removing unused client settings. Instead of
logging the full `Settings` object (which may contain sensitive topic/group
names), it now logs a summary with only `clientType`, `publishingTopicCount`,
and `subscriptionCount`.
### Changes Reviewed
- `proxy/src/main/java/.../GrpcClientSettingsManager.java` — added
`summarizeClientSettings()` helper, updated log statement
- `proxy/src/test/java/.../GrpcClientSettingsManagerTest.java` — added test
verifying no resource names leak into summary
### Assessment
**✅ Correctness** — Logic is correct. Null check is in place. The
`hasPublishing()`/`hasSubscription()` guards prevent NPE on missing optional
fields.
**✅ Performance** — No concerns. Only called in the cleanup path.
**✅ Tests** — Good test coverage. The test explicitly verifies that
sensitive topic names ("sensitive-publish-topic", "sensitive-subscribe-topic")
do not appear in the summary output, and that null input returns "null".
**✅ Compatibility** — No public API changes. Package-private static method.
### Suggestion
Same as #10721: the `summarizeClientSettings()` logic here is identical to
`summarizeSettings()` in `GrpcClientChannel` and `ClientActivity`. Consider
consolidating into a shared utility to avoid maintaining 3 copies of the same
logic.
**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]
