[ISSUE #10599]rip 2 proxy admin interface#10597
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #10597 +/- ##
=============================================
+ Coverage 48.26% 48.48% +0.21%
- Complexity 13433 13841 +408
=============================================
Files 1378 1400 +22
Lines 100817 103253 +2436
Branches 13040 13315 +275
=============================================
+ Hits 48660 50062 +1402
- Misses 46211 47131 +920
- Partials 5946 6060 +114 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR implements the M1 milestone of RIP-2 — a dedicated gRPC admin service for Proxy, decoupled from the data-plane messaging pipeline. It ships 4 core client query RPCs (listClients, listClientsByGroup, listClientsByTopic, getClientDetail) with pagination, filter pushdown, ACL 2.0 authentication, and independent port/thread pool.
Findings
-
[Info]
DefaultProxyAdminClientService.java— TheheartbeatHistoryusesConcurrentLinkedDequewith a size cap of 100 andremoveFirst(). This is correct but consider adding a comment explaining whyremoveFirst()is safe here (thesize() >= MAX_HEARTBEAT_HISTORYguard ensures non-empty deque). -
[Info]
ProxyAdminGrpcService.java:335-340— TheenforcePageSizemethod correctly caps atmaxPageSize(default 100). Consider logging a warning when the requested page size exceeds the limit, as this may indicate a client misconfiguration. -
[Info]
ProxyAdminAuthInterceptor.java— The auth interceptor correctly implements independentproxy.admin.*resource hierarchy. The separation from data-plane ACL is well-designed. One minor note: theADMIN_CODE_UNAUTHORIZEDvsADMIN_CODE_FORBIDDENdistinction is correctly applied (unauthenticated vs authenticated but no permission). -
[Warning]
DefaultProxyAdminClientService.java— ThelistClientsmethod iterates all channels fromGrpcChannelManagerfor each query. At million-connection scale this could be expensive even with filter pushdown. Consider whether a secondary index (e.g., by group or topic) would be beneficial for production deployments, or document the expected query latency at scale. -
[Info]
ProxyConfig.java— The new config fields (proxyAdminGrpcServerPort,proxyAdminMaxPageSize,proxyAdminThreadPoolNums) have sensible defaults. Port0means disabled by default which is a safe choice for backward compatibility. -
[Info]
proxy_admin.proto— Clean proto definitions with proper enum ordering and documentation. TheAdminCodeenum aligns well with the existing remoting protocol response codes.
Suggestions
- Consider adding a
@VisibleForTestingannotation or package-private constructor toDefaultProxyAdminClientServiceto make the test injection cleaner. - The PR body mentions this is M1 — it would be helpful to add a tracking issue link for subsequent milestones (M2: configuration, quota, session, diagnostic modules).
- For the
proxy_admin.proto, consider addingoption go_packageif the Go client SDK will consume these protos directly.
Cross-repo Note
The new proxy_admin.proto definitions may require corresponding updates in apache/rocketmq-clients if admin client SDKs are planned. The proto package apache.rocketmq.proxy.admin.v1 should be coordinated.
Automated review by github-manager-bot
Review by github-manager-bot (Follow-up:
|
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot (Incremental — commit 06f9980)
Summary
This commit adds the Route Change Event System and expands the admin proto surface with GetConfig/UpdateConfig, DisconnectClient, PopReceiptHandle diagnostics, and BatchConsumeDiagnostics RPCs. The route event system integrates cleanly with TopicRouteService via a new RouteRefreshListener observer pattern.
Findings
-
[Warning]
RouteChangeEventDetector.java— Each call todetectChanges()allocates a newArrayListplus intermediate maps (buildBrokerMap,buildQueueMap). Under high topic churn with hundreds of topics refreshing simultaneously (default cache refresh is every 20s), this creates short-lived object pressure. Consider: (a) accepting a pre-allocated list parameter, or (b) documenting the expected upper bound of concurrent detections. Not blocking, but worth noting for production stability. -
[Warning]
RouteChangeNotifier.java:subscribe()— The initial snapshot (sendInitialSnapshot) iterates all cached topics. If the cache has thousands of topics and a subscriber connects with an empty topic filter (subscribe to all), this could be a large synchronous operation on the gRPC thread. Consider sending the initial snapshot asynchronously via the cache refresh executor, or at least bounding the initial snapshot batch size. -
[Info]
proxy_admin.proto— TheUpdateConfigproto documents the proto3 default-value limitation (0/"" treated as "not set"). This is correct and the documentation is helpful. One suggestion: consider adding an explicitgoogle.protobuf.FieldMaskfield toUpdateConfigRequestso clients can specify exactly which fields to update, avoiding ambiguity between "intentionally set to 0" and "not set". -
[Info]
TopicRouteService.java— TheRouteRefreshListenerinterface andCopyOnWriteArrayList<RouteRefreshListener>integration is clean. The notification in the Caffeinereload()callback correctly passes both old and new views. One note: the listener notification is inside thereload()method, which means slow listeners could delay cache refresh for other topics. The current listeners (RouteChangeNotifier) do lightweight detection + async broadcast, so this should be fine in practice. -
[Info]
RouteChangeEventDetector.java— The detection logic correctly handles all 4 cases (both empty, topic create, topic delete, both have data). The broker comparison by name then by brokerId is correct. Queue scale detection covers both read and write queue num changes independently. -
[Info] Proto
ProxyRuntimeConfig— Comprehensive coverage of all proxy configuration fields (100+ fields). Field numbers up to 102 are well-organized by category. The gap in field numbers (e.g., 53-59, 71-79, 83-89) provides room for future additions without renumbering. -
[Info]
DisconnectClientRPC — Good operational capability for force-disconnecting stuck/malicious clients. Properly gated behind the admin auth interceptor. Consider adding a rate limit annotation or documentation about the operational impact (triggering rebalance for the disconnected consumer).
Suggestions
- For
RouteChangeNotifier, consider adding a max subscription limit (e.g., 10 concurrent subscribers) to prevent unbounded memory growth from abandoned streams. - The test coverage for
RouteChangeEventDetector(612 lines) andRouteChangeNotifieris thorough — good coverage of edge cases including null routes, empty routes, and multi-broker scenarios. - The new proto services (
GetConfig,UpdateConfig,DisconnectClient,DescribePopReceiptHandles,DescribeBatchConsumeDiagnostics,SubscribeRouteEvents) significantly expand the admin surface. Consider adding a versioning strategy doc for the admin proto to manage backward compatibility as the API evolves.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot (Incremental — commits 8e16613..193ccf1)
Summary
Four new commits since last review: test infrastructure refactoring (Mockito → sun.misc.Unsafe.allocateInstance() to work around Mockito 3.10 / Java 21 Byte Buddy incompatibility), new test coverage for auth/metrics/diagnostics/disconnect, and minor code cleanups.
Code Changes
-
[Info]
ProxyAdminGrpcService.java:735— Added warning log when requested page size exceedsMAX_PAGE_SIZE. Good for production debugging — operators can now see when clients request oversized pages. -
[Info]
RouteChangeNotifier.java— Removed unused imports (ArrayList,RouteChangeEvent.Builder). Clean. -
[Info]
DefaultProxyAdminClientService.java:198— Added comment explaining whyremoveLast()is safe in the eviction loop. The reasoning is correct: the while-condition guarantees the deque is non-empty. -
[Warning]
DefaultReceiptHandleManager.java:342,453— Added explicit(int)cast onhandleGroup.getHandleNum(). This makes the truncation fromlongtointexplicit, which is better than implicit narrowing. However, ifgetHandleNum()ever exceedsInteger.MAX_VALUE, this silently overflows. In practice, per-group handle counts should never reach 2B, so this is acceptable. Consider adding a precondition check or usingMath.toIntExact()if you want defensive coding:totalHandleCount += Math.toIntExact(handleGroup.getHandleNum());
Test Changes
-
[Info] Test infrastructure refactoring from Mockito to
sun.misc.Unsafe.allocateInstance()is a pragmatic solution for the Java 21 compatibility issue. TheTestTopicRouteServiceandTestServerCallStreamObserverhelper classes are well-structured. -
[Info] New test files provide good coverage:
ProxyAdminAuthInterceptorTest(+197): ACL auth, resource matching, permission checksProxyAdminMetricsManagerTest(+83): Metric registration and counter operationsDefaultProxyAdminClientServiceTest(+87): Heartbeat recording, eviction, stale client detectionProxyAdminGrpcServiceTest(+327/-30): Expanded to cover disconnect, pop receipt diagnostics, batch consume diagnostics
-
[Info]
RouteChangeNotifierTest(+250/-73): Complete rewrite with direct instantiation instead of Mockito. Tests cover subscribe/unsubscribe, initial snapshot, event broadcast, topic filtering, and backpressure.
Verdict
Incremental changes look good. The test coverage expansion is solid. The Math.toIntExact() suggestion is non-blocking.
Review by github-manager-bot (Incremental — commits 7b01e0f..1a8e7b5)SummaryTwo follow-up commits that refactor Findings
VerdictMinor deprecation warning only. The test improvement is solid. Automated review by github-manager-bot |
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot (Incremental)
Commits Reviewed
7b01e0ffix test1a8e7b5fix check
Changes
ProxyAdminGrpcServiceTest.java — Two test improvements:
-
Mock initialization: Added
@Mockannotation onadminClientServicewithMockitoAnnotations.initMocks(this)in@Before. This is cleaner than manualmock()calls and ensures consistent mock lifecycle. -
Subscribe test robustness: Replaced
verify(routeChangeNotifier).subscribe(any(), any(), any())with a realRouteChangeNotifiersubclass override that callsresponseObserver.onCompleted()and uses aCountDownLatchassertion. This tests actual behavior (the observer callback is invoked) rather than just method invocation, which is a better verification pattern.
Verdict
Both changes are test-only improvements with no production code impact. The subscribe test refactor is a meaningful quality gain — verifying behavior over invocation is the right approach for async/streaming APIs.
Automated review by github-manager-bot
PR Title
[RIP-2] Proxy Admin gRPC Interface Surface (M1: Online Client Query Module)
Summary
This PR delivers the M1 milestone of RIP-2 — the first production-ready release of the independent Proxy Admin interface surface in the
apache/rocketmqmain repository.It introduces a dedicated, standardized gRPC admin service fully decoupled from the data-plane messaging pipeline, solving the long-standing blind spot of gRPC client runtime observability. The first release ships 4 core client query RPCs with full pagination, filter pushdown, ACL 2.0 authentication and production-grade performance guarantees. It serves as the authoritative data source for RIP-1 CLIENT-01 unified client view and RIP-3
rmq.client.*tooling.1. Background & Motivation
As RocketMQ 5.0 establishes Proxy as the first-class access layer, the absence of a standardized admin interface surface creates structural gaps in operations and observability:
This PR establishes the foundational admin interface surface on the Proxy side, with client query as the first delivered module, and reserves clear extension paths for subsequent configuration, quota, session and diagnostic modules.
2. Core Design Principles
proxy.admin.*resource hierarchy with fine-grained read/write permission separation, aligned with ACL 2.0 baseline.3. Detailed Changes
3.1 Independent Admin gRPC Service
ProxyClientAdminServiceas the first admin service, with more admin services to be added under the same framework later.8082by default, completely separated from the data-plane gRPC port8081.3.2 M1 Delivery: Online Client Query APIs (4 Core RPCs)
All client data is sourced from Proxy's internal
ClientManager, which is maintained via gRPC Telemetry stream reports.ListClientsgroup,topic,clientIdPrefix,language,connectTimeStart,connectTimeEnd,pageNum,pageSizeDescribeClientclientIdListClientsByGroupgroup,pageNum,pageSizeListClientsByTopictopic,pageNum,pageSizeKey implementation details:
ClientInstanceoutput model aligned with RIP-1 CLIENT-01 specification, covering clientId, language, SDK version, protocol, access point, connection time, last active time, role, group and associated topics.DescribeClientreturns extended fields including negotiated settings, recent heartbeat history, authentication status, Pop consumption progress and network handshake information.pageSizeof 100 to prevent large queries from causing memory pressure.3.3 ACL 2.0 Dedicated Resource System
Independent admin resource hierarchy completely separated from data-plane resources:
proxy.admin.config,proxy.admin.quota,proxy.admin.sessionand other resources with corresponding read/write permissions.3.4 Performance Guarantees
ListClientspaginated query P99 latency < 1s on a single Proxy node with 1 million active connections.3.5 Built-in Observability
All admin interfaces are natively instrumented with OpenTelemetry metrics:
Metrics are uniformly exported through Proxy's existing OTel metrics endpoint and can be directly consumed by existing monitoring systems.
3.6 Documentation
4. Ecosystem Integration
With RIP-1 (Control Plane 5.0)
ProxyAdminGrpcClientandGrpcClientCollectorare fully compatible with this set of interfaces and can be switched seamlessly after release.With RIP-3 (LLM Native Integration)
rmq.client.list/rmq.client.describeCLI and MCP tools.5. Testing & Verification
6. Compatibility & Upgrade Notes
7. Acceptance Checklist
proxy_admin.protoservice definition with 4 core client query RPCsProxyAdminGrpcServiceimplementation based on ClientManagerproxy.admin.clientresource control