fix(store): refresh gRPC channels after address changes - #3130
fix(store): refresh gRPC channels after address changes#3130bitflicker64 wants to merge 3 commits into
Conversation
Refresh cached channel pools when a Store target resolves to a new address. Rebuild stale blocking and async stub pools, and guard concurrent resolution and publication races. Fixes apache#3124
218b681 to
26218cb
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the HStore gRPC client channel/stub lifecycle so that when a stable Store target (e.g., DNS name) resolves to a different address set, the client discards the prior channel pool and rebuilds related stub pools to avoid getting stuck on failed transports (issue #3124).
Changes:
- Add per-target “resolved address set” fingerprinting and retire/replace cached channel pools when the fingerprint changes (or when a previously-unresolved target first resolves).
- Rebuild blocking and async stub pools when they no longer correspond to the current channel pool, with concurrency ordering to prevent stale work from reintroducing retired channels.
- Add regression tests covering address changes and concurrent refresh/stub-build interleavings.
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 | Adds resolved-address fingerprinting, refresh/retire logic for channel pools, and stub-pool rebuild safeguards under concurrent refresh. |
| hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/client/grpc/AbstractGrpcClientTest.java | Adds refresh-focused tests validating channel replacement, stub-pool rebuild, and concurrency ordering behavior. |
| hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/client/ClientSuiteTest.java | Introduces a small suite to run the refresh regression tests together. |
Comments suppressed due to low confidence (1)
hugegraph-store/hg-store-client/src/main/java/org/apache/hugegraph/store/client/grpc/AbstractGrpcClient.java:172
- When (re)building the async stub pool, the loop always selects
targetChannels[index]for every slot, so all cached async stubs share a single channel. This defeats the channel pool and undermines concurrency/failover across channels. Bind each stub to its corresponding channel (or at least distribute across the pool) by using the loop index.
IntStream.range(0, concurrency).parallel().forEach(i -> {
ManagedChannel channel = targetChannels[index];
AbstractAsyncStub stub = getAsyncStub(channel);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| IntStream.range(0, concurrency).forEach(i -> { | ||
| ManagedChannel channel = targetChannels[index]; | ||
| AbstractBlockingStub stub = getBlockingStub(channel); | ||
| value[i] = new HgPair<>(channel, stub); | ||
| // log.info("create channel for {}",target); | ||
| }); |
There was a problem hiding this comment.
Validated as a real issue: the pool build loop used the selected request index, so every cached slot could bind to the same channel. Fixed in 95489135 by using the loop slot i for both blocking and async stub pools, and added spread assertions that the rebuilt pools cover every channel. Verified with the refreshed 9-test suite on Java 11.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Address refresh can abort active RPCs, adds synchronous DNS resolution to every request path, and does not preserve the existing gRPC target contract; the async refresh race also lacks equivalent coverage. Evidence: static review across six independent lanes; mvn test -pl hugegraph-store/hg-store-test -am -Dtest=AbstractGrpcClientTest -DfailIfNoTests=false passed 4 tests.
| if (staleChannels != null) { | ||
| Arrays.stream(staleChannels) | ||
| .filter(channel -> channel != null && !channel.isShutdown()) | ||
| .forEach(ManagedChannel::shutdownNow); |
There was a problem hiding this comment.
shutdownNow() forcibly terminates existing calls on these channels, but callers may already hold blocking or streaming stubs after the cache lock is released. A normal DNS rotation can therefore cancel active Store operations and leave writes with ambiguous outcomes. Please publish the replacement pool first, gracefully retire old channels with shutdown(), and use bounded delayed forced termination only after draining; add an active-RPC/stream refresh test.
There was a problem hiding this comment.
Fixed in 95489135. The refresh path now creates and publishes the replacement pool before retiring the old pool. Retired channels receive graceful shutdown() first, and a bounded drain task only calls shutdownNow() if termination does not complete. Added testRefreshGracefullyRetiresActiveStreamChannels() to cover a held async/stream stub during refresh.
| long resolutionRequest = resolutionRequests.computeIfAbsent(target, | ||
| key -> new AtomicLong()) | ||
| .incrementAndGet(); | ||
| String resolvedTarget = this.resolveTarget(target); |
There was a problem hiding this comment.
getChannels() call reaches this synchronous resolution path before consulting the cached pool, and Store session methods acquire a stub per request. When the JVM DNS cache expires or the resolver is slow, application request threads now block on InetAddress.getAllByName(). Please throttle/cache refreshes using a configurable interval or single-flight TTL, or refresh asynchronously while retaining the last healthy pool; cover repeated and concurrent stub acquisition with a delayed resolver.
There was a problem hiding this comment.
Fixed in 95489135. Refresh checks are now throttled by a per-target TTL and guarded with a try-lock, so only one caller performs DNS resolution while concurrent callers keep using the last cached healthy pool. Added repeated acquisition and delayed concurrent resolver coverage to verify this path.
|
|
||
| protected String resolveTarget(String target) { | ||
| try { | ||
| String host = URI.create("dns://" + target).getHost(); |
There was a problem hiding this comment.
ManagedChannelBuilder.forTarget(target). For example, a valid dns:///store:8500 target becomes dns://dns:///store:8500, so the fingerprint resolves the literal host dns; other gRPC resolver schemes can yield no host at all. The channel may still connect while refresh silently monitors the wrong endpoint. Please parse supported gRPC target schemes consistently (or explicitly validate and restrict the accepted format) and add URI-form and IPv6 target tests.
There was a problem hiding this comment.
Fixed in 95489135 for the target formats this client can safely fingerprint: raw host:port, dns:///host:port, and bracketed IPv6. Unsupported resolver schemes now return an empty fingerprint so the client does not silently monitor the wrong host while gRPC handles the original target. Broader non-DNS resolver fingerprinting is out of scope for this channel-lifecycle fix. Added URI-form, IPv6, and unsupported-scheme tests.
| } | ||
|
|
||
| @Test | ||
| public void testStubBuildRetriesAfterChannelRefresh() throws Exception { |
There was a problem hiding this comment.
getBlockingStub(), while the PR independently rewrites the async refresh/retry/cache-publication path and builds async stubs in parallel. The sequential async assertion cannot catch a stale async pool published during refresh. Please add the equivalent latch-controlled async interleaving test and assert that both returned stubs and the final cache reference only current live channels.
There was a problem hiding this comment.
Fixed in 95489135. Added testAsyncStubBuildRetriesAfterChannelRefresh() with the same latch-controlled stale-build interleaving as the blocking path, and it asserts both returned async stubs plus the final async cache only reference current live channels.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #3130 +/- ##
============================================
- Coverage 39.19% 1.54% -37.65%
+ Complexity 264 21 -243
============================================
Files 770 748 -22
Lines 65779 63264 -2515
Branches 8726 8278 -448
============================================
- Hits 25779 975 -24804
- Misses 37247 62205 +24958
+ Partials 2753 84 -2669 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- move graceful retirement to a cleanup scheduler - force-close partial pools after creation failures - validate cached stubs against channels by index - cover saturation, interruption, and drain deadlines
Purpose of the PR
HStore caches gRPC channels and blocking/asynchronous stubs by Store target.
When a stable target resolves to a different address, those pools can retain the
failed transport indefinitely even though the replacement Store is reachable.
This is the channel-lifecycle half of #3124. Complete recovery behind a stable
DNS name still requires the finite positive DNS TTL from #3126; this patch does
not bypass an indefinitely stale JVM DNS cache.
Main Changes
channel.
performs a slow DNS check while other callers keep using the cached healthy
pool.
channels with
shutdown()first and use bounded delayedshutdownNow()onlyif they do not drain.
belong to the current pool, and spread cached stubs across the channel pool.
host:port,dns:///host:port, and bracketed IPv6 targets forfingerprinting; unsupported resolver schemes are left to gRPC and skipped by
the DNS fingerprint refresh path.
cluster.
Verifying these changes
JAVA_HOME=$(/usr/libexec/java_home -v 11) mvn test -pl hugegraph-store/hg-store-test -am -Dtest=AbstractGrpcClientTest -Dsurefire.failIfNoSpecifiedTests=false:AbstractGrpcClientTestran 9 tests with 0 failures, errors, or skips on Java 11.JAVA_HOME=$(/usr/libexec/java_home -v 11) mvn test -pl hugegraph-store/hg-store-test -am -P store-client-test -Dsurefire.failIfNoSpecifiedTests=false:ClientSuiteTestran 9 tests with 0 failures, errors, or skips on Java 11.JAVA_HOME=$(/usr/libexec/java_home -v 11) mvn editorconfig:format: passed with 0 files changed.JAVA_HOME=$(/usr/libexec/java_home -v 11) mvn clean compile -Dmaven.javadoc.skip=true: passed across all 38 reactor modules on Java 11.Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need