lizhimins commented on PR #4062: URL: https://github.com/apache/rocketmq-dashboard/pull/4062#issuecomment-5581511118
Closing this. The blocking problem is not a matter of taste or of missing tests — the PR cannot be built by anyone except its author, and the capability it needs does not exist in any Apache release. There is also already a supported path on trunk that reaches the same data with no new dependency at all. ## 1. The declared dependency does not contain the RPCs this code calls `Rip2ProxyAdminClient` imports `apache.rocketmq.v2.ListConsumerConnectionRequest`, `ListConsumerConnectionResponse`, `ListSubscriptionRequest`, `ListSubscriptionResponse`, `SubscriptionInfo` and `ClientInfo`, and calls `stub.listConsumerConnection(...)` and `stub.listSubscription(...)`. We resolved the real artifact from Maven Central — `org.apache.rocketmq:rocketmq-proto:2.2.0`, sha1 `b064dc3bcbf498a47a8ae9c872824d0a5d1ba7bc`, matching Central's published checksum, and `maven-metadata.xml` confirms 2.2.0 is the latest release — and inspected it: - **None of those class names exist in the artifact.** - Its bundled `apache/rocketmq/v2/admin.proto:41-42` declares `service Admin` with exactly **one** RPC: `ChangeLogLevel`. - `service.proto:353` (`MessagingService`) has no such RPC either. So the proxy-side admin surface this PR talks to exists only in an unreleased personal fork. The POM comments say as much: *"pin grpc to a locally available version; rocketmq-proto's transitive grpc 1.45 poms are pom-only in the offline repository"* and *"Offline-repo availability pins for grpc's transitive graph"*. An ASF project's POM has to resolve from Central; one contributor's offline mirror cannot define the project's dependency graph. Relatedly, port `8083` appears nowhere in this repository. The build result quoted in the description (`mvn -o test`, 2032/2035 passing) is only reproducible on the author's machine, which is why our own CI could not confirm it — note that `ci.yml` currently fails at startup and reports no checks on any PR, so nothing here was verified by automation either way. ## 2. Trunk already reaches proxy-connected gRPC clients, with zero new dependencies This is the more important half. `ProxyConsumerResolver` already does the job: - Its Javadoc (`:45-53`) states that gRPC clients register through gRPC heartbeats and are invisible to broker stats — the exact gap this PR set out to close. - `queryProxy` (`:97-107`) issues `GET_CONSUMER_CONNECTION_LIST` against the proxy's **remoting** port 8080, which the proxy answers from its own client manager. - Proxy hosts are discovered automatically via the heartbeat-syncer group with a TTL cache (`:150-185`), already pooled through `MqAdminExtFactory` / `RuntimeAdminClientResolver` (`:187-192`). The actual gap is much smaller than this PR assumes: `RocketMQClientProvider.findConsumerConnections` (`:315-355`) queries only the broker and never consults `ProxyConsumerResolver`, and `toConnectionVO` (`:383-395`) hardcodes `.protocol(Protocol.Remoting)`. Routing that method through the existing resolver — and stopping the hardcoded protocol — gets gRPC clients into the client view with **no new dependency, no new port, and no new configuration surface**. The frontend needs nothing: `web/src/pages/cluster/clients.tsx:66-67`, `web/src/constants/theme.ts:68` and `ClientsPage.test.tsx:76,192` already handle and assert gRPC rows. ## 3. Even setting compilation aside, the runtime behaviour would regress the existing endpoint - **It is not inert by default.** With `proxy-admin-addresses` empty it falls back to `knownProxyAddresses()`, whose initial value is a hardcoded `127.0.0.1:8081` (`ProxyAddressService.java:83`), deriving `127.0.0.1:8083`. So on a default install **every** `/api/clients` call dials a dead localhost port. - **Serial 3s deadlines** across all candidate addresses add N×3s to a listing that is currently fast. Compare `ProxyAddressService.buildTopology` (`:75-80`, `:157-165`), which parallelises on a bounded executor with a hard 10s cap. - **`catch (Throwable t)` is too broad** — it swallows NPEs and Errors. In the existing `ClientServiceTest:37-42` (`@Mock ClientProvider` + `@InjectMocks`) the new collaborator is null, so the NPE is caught and the base list returned: the suite stays green while covering nothing. - **The `clusterId` filter is not honoured.** Merged rows are simply stamped `clusterName(clusterId)` with no check that the gRPC client belongs to that cluster, so `?clusterId=X` would return foreign clients labelled X. - **Silent degradation.** New rows are built `.partial(false)` and the result is never marked degraded, so an operator reads a truncated list as complete. - **`mapLanguage` fabricates values.** Prefix matching over `ClientLanguage` maps "P" to Python and "C" to Cpp; rocketmq-apis' `NODE_JS` never matches `NodeJS`; unknown or blank defaults to **Java**, whereas trunk's `RocketMQClientProvider.mapLanguage:415-418` returns `null`. - **No ACL support at all** — `usePlaintext()`, no auth interceptor, TLS not configurable — so it cannot talk to any secured proxy. - **Channels are never evicted**: `ProxyAddressService.removeProxyAddr` (`:282-290`) leaves a live netty channel until process shutdown. On pooling: caching channels in a `ConcurrentHashMap` with `@PreDestroy` shutdown is better than per-request creation, but it is not this project's regime. `MqAdminExtFactory` keys on `AdminClientCacheKey(namesrvAddr, authenticationIdentity)` (`:227`) with `release` (`:117`, `:136`); `MqClientPool` keys on `ClientKey(namesrvAddr, authenticationIdentity, kind)` (`:50`). An address-only map has no credential identity and no release hook, so pooled channels survive a credential rotation. ## 4. It would not work in the deployment we document `deploy/docker-compose.yml` defines only mysql, rocketmq-server and rocketmq-web — there is **no proxy service**, and `STUDIO_ROCKETMQ_PROXY_ADMIN_ADDRESSES` is not in the env block (`:38-55`). This is the same shape as #3133 (discover NameServers from Kubernetes), which we returned for the same reason: a heavy new external client plus a new port, unusable in the deployment form we actually document and test. ## Smaller items, for reference - The `<dependencyManagement>` block is not needed. `guava 32.0.1-jre` is a no-op (`rocketmq-tools:5.5.0` already declares that exact version at depth 1, and Spring Boot does not manage guava); `protobuf-java 3.24.0` is redundant (`grpc-protobuf:1.59.1` already brings it); `protobuf-javalite 3.19.2` is dead and would be a version-skew hazard if anything ever pulled it, since `grpc-protobuf` explicitly excludes javalite; and `javax.annotation-api:1.3.2` is unnecessary — the generated stubs carry `io.grpc.stub.annotations.GrpcGenerated`, and rocketmq-proto depends on `jakarta.annotation-api`. Only the `grpc-bom` import addresses a real gap. - `rocketmq-proto` is declared twice in `<dependencies>`, both times with a hardcoded `2.2.0`. - `javax.annotation` adds a legacy namespace to a Jakarta-only Spring Boot 3 / Java 21 application. - 331 new lines with no tests; `ClientServiceTest` was not updated. - Hand-written constructor with `@Value` instead of `@RequiredArgsConstructor`; `knownProxyAddresses()` uses fully-qualified `java.util.List`/`ArrayList` although the file imports both (`:152`, `:169`); `ProxyAddressService` is fully qualified three times; `ObjectProvider` is used without a cycle to justify it (`ProxyAddressService`'s own deps are at `:91-93`). - The branch also carries unrelated commits. One note: the automated `RockteMQ-AI` review on this PR is APPROVED, but it cites `ProxyAdminGrpcClient.java`, `ProxyAdminGrpcClientTest.java` and merge logic at `ClientController.java:67-110` — none of which exist in this diff. Please do not treat that bot's approval as a signal; it approves unconditionally and here it hallucinated the file names. ## What we would merge instead A PR that routes `RocketMQClientProvider.findConsumerConnections` through the existing `ProxyConsumerResolver` and stops hardcoding `Protocol.Remoting` in `toConnectionVO`. That surfaces gRPC SDK clients in the client view, needs no new dependency or port, works in the documented docker-compose deployment, inherits the existing pooling and proxy auto-discovery, and would be a small diff we could review and merge quickly. If you want to take that on, we are glad to review it. -- 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]
