fix(store): refresh gRPC channels after address changes - #3130
fix(store): refresh gRPC channels after address changes#3130bitflicker64 wants to merge 4 commits into
Conversation
dosubot
Bot
added
size:XL
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.
imbajin
left a comment
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.
dosubot
Bot
added
size:XXL
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3130 +/- ##
============================================
+ Coverage 39.13% 43.62% +4.49%
- Complexity 264 546 +282
============================================
Files 770 781 +11
Lines 65779 66926 +1147
Branches 8726 8923 +197
============================================
+ Hits 25742 29199 +3457
+ Misses 37288 34684 -2604
- Partials 2749 3043 +294 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
- 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
a08ac4b to
ddeef7a
Compare
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: DNS refresh can fail under the default security policy, and QueryV2 can race channel retirement. Evidence: six independent exact-head lanes; 14/14 focused Java tests and git diff --check passed; all visible exact-head checks passed.
| return ""; | ||
| } | ||
| try { | ||
| return Arrays.stream(this.resolveHost(host)) |
There was a problem hiding this comment.
getChannels(), but InetAddress.getAllByName() invokes SecurityManager.checkConnect(host, -1). The default launcher enables HugeSecurityManager, whose checkConnect() rejects Gremlin worker stacks and has no HStore/gRPC exemption; only UnknownHostException is caught here. An HStore-backed Gremlin request can therefore throw SecurityException on initial resolution or a periodic refresh instead of using the healthy pool. Please move resolution to a trusted bounded background path (preserving the current pool on failure), or add the narrowest safe HStore policy exemption, and cover a real Gremlin-worker + HugeSecurityManager lookup regression.
There was a problem hiding this comment.
Fixed in 198de19e. resolveTarget(), replacement creation and retirement now run on a prestarted channel-maintenance executor; getChannels() only triggers a refresh and keeps serving the last healthy pool. I avoided a HugeSecurityManager exemption: getChannels(String) was public, and the lazy thread creation in retireChannels()'s schedule() would trip checkAccess(ThreadGroup) under the same predicate anyway. Covered by testRefreshSucceedsWhenTheCallerThreadIsDeniedSocketAccess(), which denies checkConnect/checkAccess on gremlin-server-exec-* threads and asserts the caller still gets a stub and all resolution ran on maintenance threads.
One pre-existing gap I'd file separately: the very first pool for a cold target is still built via the caller, so a Gremlin worker that is the first caller for a new address can still be denied there.
| } | ||
|
|
||
| if (replaced) { | ||
| this.retireChannels(staleChannels); |
There was a problem hiding this comment.
tryLock() can still receive staleChannels; after this swap, retirement immediately calls shutdown() on that pool. QueryV2Client#getManagedChannel() uses getChannels() directly and creates its async stub only afterwards, bypassing the identity/retry loops added to getAsyncStub() and getBlockingStub(), so a refresh race can hand QueryV2 a channel that rejects the new RPC. Please route QueryV2 through the guarded async-stub path or provide an atomic channel/stub lease, and add an interleaving test for this production consumer.
There was a problem hiding this comment.
Fixed in 198de19e. getQueryServiceStub() now returns getAsyncStub(target) and getManagedChannel() is gone, so QueryV2 uses the same identity/retry loop as every other consumer. The hand-rolled round-robin only existed because the async pool once bound every entry to channels[index] (fixed in #3128), so nothing is lost. getChannels(String) is now protected — handing out a raw ManagedChannel[] is unsafe once refresh can retire it. Covered by testQueryV2StubFollowsPublishedPoolAcrossRefresh(), which refreshes the pool while a stub build is blocked mid-flight; it fails against the old body.
Note: routing narrows the race but a refresh landing between stub return and first call can still hit a just-shut-down channel — retry-on-UNAVAILABLE would close it, better as a follow-up.
Channel refresh resolved DNS on whichever thread asked for a stub. Under the default launcher that thread can be a Gremlin worker, and HugeSecurityManager denies it socket access, so an HStore-backed request could fail with a SecurityException instead of using the healthy pool. - run resolution, replacement creation and retirement on a channel maintenance executor, keeping the last healthy pool when resolution fails or times out - build the first pool for a target once its address is known, so a cold start no longer creates and immediately retires a pool - replace the per-target refresh lock with a single-flight task map that the cold path can also wait on, and throttle from both submission and completion - route QueryV2Client through the guarded async stub path instead of taking a channel straight from the pool, and restrict getChannels to subclasses - drop the channel monitor from stub acquisition: publishing a pool before retiring the previous one already orders the check, and the monitor is static - log refresh failures and pool replacements, which the executor otherwise discards, and never let a denied thread creation wedge refresh for a target - parse targets with URI, rejecting resolver schemes such as unix:/path that were resolved as a host named after the scheme - fold the blocking and async stub acquisition loops into one implementation
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. The initial refresh deadline can suppress DNS monitoring for the lifetime of a target when the JVM's nanoTime origin is negative; three independent current-head review lanes converged on this issue.
| private boolean shouldRefreshChannels(String target) { | ||
| AtomicLong nextResolution = nextResolutions.computeIfAbsent(target, | ||
| key -> new AtomicLong()); | ||
| return System.nanoTime() - nextResolution.get() >= 0L; |
There was a problem hiding this comment.
System.nanoTime() has an arbitrary origin and may be negative. In that case this comparison is false for a newly seen target; because postponeNextRefresh() only writes a real deadline after a refresh is submitted, this target can keep serving its initial pool without ever recording or checking the resolved address until the clock crosses zero. Please represent the first run explicitly (rather than using zero as a timestamp), keep the elapsed-time comparison overflow-safe, and add a regression with an injectable negative nanoTime() value.
…tore disabled The Server wrapper now writes auth.admin_pa from the auth Secret alongside usePD and pd.peers, so an auth-enabled release keeps its configured admin password with init_store.enabled=false instead of silently falling back to the public default. The Secret value is rejected when it contains properties-parser metacharacters that would inject config lines or store a different password than the Secret holds. The new hubble component deploys the Hubble UI as a single-replica Deployment with pd and direct wiring modes, optional Ingress and H2 persistence, schema validation, render-time guards, docs, and CI coverage. PD-meta installs (auth enabled, or Hubble in pd mode) also announce the Server client Service URL to PD via server.urls_to_pd and server.deploy_in_k8s so discovery clients receive a resolvable address instead of the 0.0.0.0 default, and the Hubble wrapper writes server.host so current images bind all interfaces. Because current Hubble images authenticate their login against the cluster, rendering Hubble without server.auth fails unless explicitly overridden. The CI invalid-value step now fails on every case rather than only its last line, and positive renders cover both Hubble modes. Validated against a composition of master 1716c77 plus the current heads of apache#3119 (edf07d0), apache#3126 (b40c42f), and apache#3130 (198de19): fresh auth-enabled installs reach Ready with zero restarts, the admin credential comes from the Secret while unauthenticated and default-password requests get 401, and Hubble logs in with the Secret credential and reads cluster metadata through PD discovery, with its H2 metadata persisted on the PVC.
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