[Studio] feat: Feature/rip 2 pr1 proto serialization - #10702
Conversation
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR establishes the foundational proto schema and serialization layer for the RIP-2 Proxy Admin gRPC service (PR#1 of a 6-PR sequence). It introduces proxy_admin.proto (606 lines, 10 RPCs), ProxyAdminMarshaller, ProxyAdminProtoConverter, and AdminCode — along with Maven/Bazel build integration and comprehensive unit tests.
Overall the design is clean and well-structured. Below are observations for the author's consideration.
Findings
-
[Warning]
proxy_admin.proto— Missing newline at end of file. The diff shows\ No newline at end of file. Most editors and CI lint checks expect a trailing newline. -
[Warning]
proxy_admin.proto(UpdateConfigRequest) — The comment acknowledges proto3 default-value semantics prevent toggling booleans OFF (true→false). Consider usinggoogle.protobuf.BoolValue(wrapper type) for boolean config fields, orgoogle.protobuf.FieldMaskto explicitly specify which fields to update. This avoids a surprising limitation for API consumers. -
[Warning]
proxy_admin.proto(ProxyRuntimeConfig) — Field numbers go up to 102 with intentional gaps (1-12, 20-29, 30-39, 40-49, 50-53, 60-70, 80-82, 90-92, 100-102). If the gaps are reserved for future use, consider adding// Reserved for future: 13-19, 54-59, ...comments to make the intent explicit and prevent accidental reuse. -
[Warning]
proxy_admin.proto(SubscribeRouteEvents) — Server-streaming RPC with no documented keepalive, deadline, or reconnection strategy. If the proxy stops sending events, the stream could hang indefinitely. Consider documenting expected keepalive behavior or adding aheartbeat_interval_secondsfield toSubscribeRouteEventsRequest. -
[Info]
ProxyAdminProtoConverter.java— At 1016 lines, this class is quite large. Consider splitting into focused converters (e.g.,ClientProtoConverter,RouteProtoConverter,ReceiptHandleProtoConverter,ConfigProtoConverter) as subsequent PRs add more RPC implementations. Not blocking for this PR. -
[Info]
ProxyAdminMarshaller.java:parse()— WhentypeUrldoesn't match any known message, the method logs a warning and returnsnull. Callers must null-check everyparse()result. Consider throwingInvalidProtocolBufferExceptioninstead, which is more idiomatic for protobuf deserialization and forces explicit error handling. -
[Info]
AdminCode.javavsproxy_admin.protoAdminCodeenum — Both define anAdminCodeconcept at different levels (Java error code vs proto enum). The naming overlap could cause confusion during development. Consider renaming the Java class toAdminErrorCodeorAdminResponseCodeto differentiate.
Positive Observations
- ✅ Proto schema follows proto3 best practices:
UNSPECIFIED = 0for all enums,java_multiple_files = true, proper package naming - ✅ Null-safe conversion pattern consistently applied (
field != null ? field : "") - ✅ Good test coverage:
AdminCodeTest,ProxyAdminMarshallerTest,ProxyAdminProtoConverterTestcover edge cases including null inputs, empty lists, and round-trip serialization - ✅ Checkstyle suppressions properly scoped to generated code only
- ✅ Both Maven and Bazel build paths covered
Cross-repo Note
This PR defines the proto schema under apache.rocketmq.proxy.admin.v1. When the client-side admin SDK is implemented, apache/rocketmq-clients may need corresponding proto definitions or a shared proto artifact.
Automated review by github-manager-bot
c9a981e to
50faf99
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10702 +/- ##
=============================================
+ Coverage 48.31% 48.43% +0.11%
- Complexity 13511 13650 +139
=============================================
Files 1380 1392 +12
Lines 101091 101465 +374
Branches 13101 13102 +1
=============================================
+ Hits 48844 49144 +300
- Misses 46285 46345 +60
- Partials 5962 5976 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
50faf99 to
813eeae
Compare
…dels - Add proxy_admin.proto with ProxyAdminService gRPC definitions - Add protobuf-maven-plugin and bazel proto rules for code generation - Add ProxyAdminMarshaller for gRPC method marshalling - Add admin domain models (client info, route change, diagnostics) - Add AdminCode error codes and ProxyConfig admin options - Add SSL/auth context variables to ProxyContext - Exclude generated sources from checkstyle
813eeae to
bfa485f
Compare
…ed TLS handshake The four handshake-rejection cases only caught RemotingSendRequestException, but on slow CI runners the channel may be closed by the failed handshake before the request is flushed, surfacing as RemotingConnectException instead. Both exceptions prove the connection was rejected.
…ection margin waitTimeoutMs=20/threshold=18 left only 2ms of headroom, so any CI scheduler or GC pause >=18ms was misreported as a lost wakeup. A delivered wakeup returns in microseconds while a lost one blocks for the full interval, so a 2000ms interval with a 1000ms threshold keeps the two cases distinguishable and immune to scheduling jitter.
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR introduces the Proxy Admin gRPC protocol definition, serialization layer, and domain models for RIP-2 (RocketMQ Studio/Admin). It adds proxy_admin.proto (606 lines), domain models, and a custom ProxyAdminMarshaller.
Findings
[Positive] proxy_admin.proto — Well-structured proto definition with clear message types for admin operations (client queries, diagnostics, config management). Good use of enums for admin codes.
[Positive] ProxyAdminMarshaller.java — Custom gRPC marshaller using JSON serialization. Clean implementation with proper charset handling (StandardCharsets.UTF_8).
[Positive] Domain models — ClientDetailInfo, ClientInstanceInfo, RouteChangeEvent, TopicRouteSnapshot etc. are well-structured with proper builders and toString methods.
[Positive] Test coverage — Good unit tests for BatchConsumeClientDiagnostics, BatchConsumeGroupSummary, PopReceiptHandleGroupSummary, and PopReceiptHandleInfo.
[Warning] WORKSPACE:161-175 — Adding rules_proto 5.3.0-21.7 via http_archive. The comment mentions avoiding Bzlmod requirement from 6.0.0 — this is fine, but consider pinning the exact version in a comment for future maintainers.
[Info] ProxyConfig.java — Adding 98 lines of new config fields. Consider grouping related admin config fields with comments for readability.
Suggestions
- The proto file is large (606 lines). Consider splitting into multiple
.protofiles by domain (e.g.,admin_client.proto,admin_config.proto) if it continues to grow. - Ensure
ProxyAdminMarshallerhandles deserialization errors gracefully (currently usesObjectMapper.readValuewhich can throw).
Cross-repo Note
This PR adds proto definitions that may require corresponding updates in apache/rocketmq-clients if the admin API needs to be consumed by external tools.
Automated review by github-manager-bot
RIP-2 Proxy Admin Interface — PR#1 Foundation: gRPC Proto, Domain Models & Serialization
Background
This PR delivers the foundational protocol definition, domain-model layer, and serialization infrastructure for the Proxy Admin gRPC service under RIP-2 (Proxy Admin Interface). It is the first atomic PR of a six-PR sequence that fully implements the Proxy Admin capability suite.
Scope note: this PR establishes the contract and data structures only. The gRPC service handlers, the auth/authorization interceptor, and the domain↔proto converter (
ProxyAdminProtoConverter) are delivered in subsequent PRs (PR2 / PR3 / PR4).Design Overview
1. gRPC Protocol Definition (
proxy_admin.proto)A new gRPC service
ProxyClientAdminService(notProxyAdminService) is defined, exposing 10 RPC endpoints:ListClients,DescribeClient,ListClientsByGroup,ListClientsByTopicDescribePopReceiptHandles,DescribeBatchConsumeDiagnosticsGetConfig,UpdateConfigDisconnectClientSubscribeRouteEvents— server-streaming, pushes incremental route change events2. Domain Model Layer
Internal models decouple handlers from raw protobuf and isolate schema changes:
proxy/common/:ProxyContext,ContextVariable,PopReceiptHandleInfo,PopReceiptHandleGroupSummary,BatchConsumeClientDiagnostics,BatchConsumeGroupSummaryproxy/grpc/admin/model/:ClientDetailInfo,ClientInstanceInfo,ListClientsFilter,RouteChangeEvent,RouteChangeEventType,TopicRouteSnapshot3. Serialization & Error Code
ProxyAdminMarshaller— custom gRPCMarshallerhandling nullable fields and unrecognized/unknown enum values.AdminCode— global standardized error-code enum unifying error responses across Proxy Admin APIs.(The bidirectional converter
ProxyAdminProtoConverteris intentionally not in this PR; it lands in PR4 with the service handlers.)4. Configuration Scaffolding (
ProxyConfig)8 new
proxyAdmin*fields —proxyAdminEnabled(default true),proxyAdminServerPort(8082),proxyAdminThreadPoolNums(4),proxyAdminMaxPageSize(100),proxyAdminDescribeClientConcurrencyLimit(8),proxyAdminSamplingRateUnderLoad(0.5),proxyAdminHeartbeatHistorySize(10),proxyAdminSamplingThreshold(100000).5. Build & Code Style
proxy/pom.xml— addprotobuf-maven-plugin+os-maven-plugin+maven-checkstyle-pluginconfig (no new<dependency>).proxy/BUILD.bazel— addjava_proto_librarytarget for the admin proto.WORKSPACE— addrules_protoexternal repository (+16 lines).style/rmq_checkstyle.xml+style/rmq_checkstyle_suppressions.xml— extend checkstyle rules.Modified & New File Scope
Protocol / serialization / models (new)
proxy/src/main/proto/proxy_admin.protoProxyClientAdminService+ 10 RPCs, 605 linesproxy/.../grpc/admin/ProxyAdminMarshaller.javaproxy/.../grpc/admin/AdminCode.javaproxy/.../grpc/admin/model/*.java(6 files)proxy/.../common/*.java(6 files)Config / build / style
proxy/.../config/ProxyConfig.javaproxyAdmin*fieldsproxy/.../grpc/v2/client/ClientActivity.javaproxy/pom.xml,proxy/BUILD.bazel,WORKSPACEstyle/rmq_checkstyle.xml,style/rmq_checkstyle_suppressions.xmlTests (16 new/modified) —
ProxyAdminMarshallerTest(288 lines, round-trip + nullable + unknown-enum),AdminCodeTest, all 6admin/model/*Test, 5proxy/common/*Test,ProxyConfigTest,ClientActivityTest.Unrelated (recommend splitting out) —
common/.../ServiceThreadTest.java,remoting/.../TlsTest.java(flaky-test fixes, not Proxy Admin).Total: 38 files (+3,399 / −19), of which 36 are PR1 content and 2 are unrelated.
Dependencies
No new Maven
<dependency>. Addsrules_proto(Bazel) and protobuf/grpc build plugins that generate stubs at compile time.Test Coverage & Validation
Marshaller round-trip + nullable/unknown-enum edge cases; AdminCode mapping; full coverage for all domain models; ProxyConfig defaults; adjusted ClientActivityTest.
Follow-ups / Reviewer Notes (must address)
ProxyClientAdminServicewith 10 RPCs, notProxyAdminServicewith 6. The write operations (UpdateConfig,DisconnectClient) were omitted from the original draft and are called out here.proxyAdminEnableddefaults totrue— risky with no auth yet (PR3). Recommend defaulting tofalse.BUILD.bazelhas onlyjava_proto_library(nojava_grpc_library), andprotobuf-maven-pluginis not configured with thecompile-customgoal +protoc-gen-grpc-java.ProxyClientAdminServiceGrpcwill not be generated for PR2 — verify/fix.AdminCodeproto/Java drift —AdminCode.javagoes to code 8; proto'sAdminCodeenum tops out at 7 (ADMIN_CODE_TOO_MANY_REQUESTS). Align before PR4 converter.rmq_checkstyle_suppressions.xmlis referenced by no pom (suppressionsLocationmissing). Wire it up or drop it.ServiceThreadTest/TlsTestinto their own PR.Development Branch
feature/rip-2-pr1-proto-serializationRelated Tracking
RIP-2: Proxy Admin Interface — Step 1 / 6.