RockteMQ-AI commented on PR #11051: URL: https://github.com/apache/rocketmq/pull/11051#issuecomment-5557484313
bash: line 166: develop: command not found bash: line 166: Proxy: command not found bash: line 166: Remoting: command not found bash: line 166: Proxy: command not found bash: line 166: ClientManagerActivity: command not found bash: line 166: HEART_BEAT: command not found bash: line 166: UNREGISTER_CLIENT: command not found bash: line 166: CHECK_CLIENT_CONFIG: command not found bash: line 166: ClientActivity: command not found bash: line 166: heartbeat: command not found bash: line 166: notifyClientTermination: command not found bash: line 166: Proxy: command not found bash: line 166: RocketMQ: command not found bash: line 166: Cluster: command not found bash: line 166: Local: command not found bash: line 166: Pop: command not found Verification complete — all claims check out against the code. Here is the review. --- ## Code Review: PR #11051 — proxy-client-management-docs ### 1. Summary Docs-only change (+10/−0, `proxy/README.md`): adds a "Client management" H2 section between the introduction and "Architecture", describing how Proxy tracks online client connections across both protocol paths — remoting via `ClientManagerActivity` and gRPC v2 via `ClientActivity` — and linking that capability to the control-plane direction of the RocketMQ 5.0 Studio proposal. No code, protocol, or behavioral changes; zero risk to the message path, broker, store, or `rocketmq-clients` compatibility. ### 2. Potential Issues No functional issues. I verified the technical claims against `develop`: - `ClientManagerActivity` (proxy/src/main/java/org/apache/rocketmq/proxy/remoting/activity/ClientManagerActivity.java:70-77) does dispatch exactly `HEART_BEAT`, `UNREGISTER_CLIENT`, `CHECK_CLIENT_CONFIG`. Accurate. - `ClientActivity` (proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/client/ClientActivity.java) does implement `heartbeat`, `NotifyClientTermination`, and `SyncLiteSubscription` handling. Accurate. - "Same lifecycle on both paths" is fair: both activities register `ConsumerIdsChangeListener`/`ProducerChangeListener` and feed `ClientChannelInfo` through their channel managers. Minor documentation concerns: - **Unlinked reference** (proxy/README.md:34, new lines): "the RocketMQ 5.0 Studio proposal" is cited without a link. The proposal exists (issue #10600 / Proxy Admin interface #10825), but an unlabeled reference will rot and readers can't find it. - **Vague terms** (proxy/README.md:31-32): "lite subscription sync" should be the actual RPC name `SyncLiteSubscription` for greppability; "admin-style inspection" names nothing concrete (e.g., `GET_CONSUMER_CONNECTION_LIST`). - **PR hygiene**: template is unfilled (`Fixes #issue_id`), no issue linked, empty test section. Minor for docs, but the contribution guide expects issue linkage. ### 3. Code Quality Good. Heading style (sentence-case H2) and backtick-identifier usage match the rest of the file; placement before "Architecture" is logical; prose is concise. ### 4. Suggestions 1. Link the proposal: "...described in the [RocketMQ 5.0 Studio proposal](https://github.com/apache/rocketmq/issues/10600)". 2. Use concrete identifiers: `SyncLiteSubscription` instead of "lite subscription sync"; name the actual admin operations instead of "admin-style inspection". 3. Link an issue in the PR description per the template. **Verdict**: Accurate, low-risk docs change — approve with the minor nits above. -- 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]
