You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "Aias00 (via GitHub)" <gi...@apache.org> on 2026/07/31 08:08:38 UTC
[PR] [ISSUE #10718] Summarize removed client settings logs [rocketmq]
Aias00 opened a new pull request, #10719:
URL: https://github.com/apache/rocketmq/pull/10719
## Summary
- replace full gRPC Settings logging in the unused client settings cleaner with a compact summary
- log the actual consumer group name and clientId for diagnostics
- add test coverage to ensure publish/subscription resource names are not exposed by the summary
Fixes https://github.com/apache/rocketmq/issues/10718
## Tests
- JAVA_HOME=$(/usr/libexec/java_home -v 1.8) mvn -pl proxy -Dtest=GrpcClientSettingsManagerTest -DfailIfNoTests=false test
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10718] Summarize removed client settings logs [rocketmq]
Posted by "RockteMQ-AI (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: harden GrpcClientSettingsManager offline cleanup diagnostics [rocketmq]
Posted by "Aias00 (via GitHub) via commits" <co...@rocketmq.apache.org>.
Aias00 closed pull request #10719: fix: harden GrpcClientSettingsManager offline cleanup diagnostics
URL: https://github.com/apache/rocketmq/pull/10719
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10718] Summarize removed client settings logs [rocketmq]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10719:
URL: https://github.com/apache/rocketmq/pull/10719#issuecomment-5141199470
## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/10719?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `50.00000%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 48.22%. Comparing base ([`00e45b8`](https://app.codecov.io/gh/apache/rocketmq/commit/00e45b8a6db23efbe756d0306f10716156cfd4dd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`731228a`](https://app.codecov.io/gh/apache/rocketmq/commit/731228ac3e0a833e0aa3c4469913be5584913c6c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/rocketmq/pull/10719?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...roxy/grpc/v2/common/GrpcClientSettingsManager.java](https://app.codecov.io/gh/apache/rocketmq/pull/10719?src=pr&el=tree&filepath=proxy%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fproxy%2Fgrpc%2Fv2%2Fcommon%2FGrpcClientSettingsManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHJveHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3Byb3h5L2dycGMvdjIvY29tbW9uL0dycGNDbGllbnRTZXR0aW5nc01hbmFnZXIuamF2YQ==) | 50.00% | [3 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/10719?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## develop #10719 +/- ##
=============================================
- Coverage 48.31% 48.22% -0.09%
+ Complexity 13511 13481 -30
=============================================
Files 1380 1380
Lines 101091 101098 +7
Branches 13101 13103 +2
=============================================
- Hits 48844 48759 -85
- Misses 46285 46355 +70
- Partials 5962 5984 +22
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/rocketmq/pull/10719?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org