fix(store): bind each gRPC stub to its own channel - #3128
Conversation
AbstractGrpcClient opens concurrency (32) ManagedChannels per target, but both stub-pool initializers used one precomputed channel index inside their loops. Every pool entry therefore pointed at the same channel while the other 31 channels remained idle. - use channels[i] in the blocking and async pool initializers - add self-contained tests that verify every pool channel is bound - add ClientSuiteTest so the store-client-test profile runs the tests Fixes apache#3125
There was a problem hiding this comment.
Pull request overview
This PR fixes a channel-indexing bug in AbstractGrpcClient where both blocking and async stub pools were unintentionally binding all entries to a single ManagedChannel, preventing the intended distribution across the per-target channel pool. It also adds targeted unit tests (run via the existing store-client-test surefire execution) to ensure each stub-pool entry is bound to its corresponding channel.
Changes:
- Fix stub-pool initialization to bind
value[i]tochannels[i]for both blocking and async stubs. - Add unit tests with fake channels/stubs to verify both pools cover all created channels without requiring a running cluster.
- Add a
ClientSuiteTestsuite to ensurestore-client-testruns the new coverage while excluding cluster-dependent client tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hugegraph-store/hg-store-client/src/main/java/org/apache/hugegraph/store/client/grpc/AbstractGrpcClient.java | Corrects stub-pool initialization so each stub is paired with its corresponding channel index. |
| hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/client/grpc/AbstractGrpcClientTest.java | Adds unit tests to assert blocking/async stub pools span all channels using local fakes. |
| hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/client/ClientSuiteTest.java | Adds the store-client-test suite entry point to run only cluster-independent client tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Override | ||
| public boolean awaitTermination(long timeout, TimeUnit unit) { | ||
| return this.shutdown; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
@copilot These methods are concrete in gRPC 1.39.0, not abstract. FakeManagedChannel implements all required abstract methods, and both the Java 11 store-client-test profile and the Store CI compile and pass. No additional overrides are required.
Purpose of the PR
AbstractGrpcClientcreates 32 managed channels per target, but bothstub-pool initialization loops bound every entry to one channel selected
before the loop. Traffic was concentrated on a single connection while the
other 31 channels remained idle.
Main Changes
cover every channel without a running PD or Store cluster.
ClientSuiteTestso the existingstore-client-testprofile runs thenew coverage without including cluster-dependent client tests.
Verifying these changes
mvn install -pl hugegraph-struct -am -DskipTests: passed.mvn test -pl hugegraph-store/hg-store-test -am -P store-client-test: 2 passed, 0 failed.mvn editorconfig:format: no files changed.mvn clean compile -Dmaven.javadoc.skip=true: passed on Java 11.Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need