fix(pd): keep KvClient watches alive after reconnect failures - #3157
fix(pd): keep KvClient watches alive after reconnect failures#3157contrueCT wants to merge 2 commits into
Conversation
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The reconnect implementation has critical identity and failure-propagation risks that can silently stop watches and invalidate existing locks; permanent callback errors also retry without termination. Evidence: static review of KvClient.java and AbstractClient.java at head 772b3a5, with exact server lock/watch handling cross-checked.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3157 +/- ##
============================================
- Coverage 39.23% 38.34% -0.89%
- Complexity 264 424 +160
============================================
Files 771 771
Lines 65938 66024 +86
Branches 8759 8778 +19
============================================
- Hits 25872 25320 -552
- Misses 37310 37970 +660
+ Partials 2756 2734 -22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The current head addresses the previously reported watch reconnect, lock identity, terminal-error retry, and stale-observer issues; focused validation passes, but coverage checks remain failed and independent review evidence is incomplete. Evidence: mvn -pl hugegraph-pd/hg-pd-test -am -Dtest=KvClientTest -DfailIfNoTests=false test (18/18 passed), git diff --check passed; codecov/project and codecov/patch failed.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Splitting the lock and watch client IDs, tracking each watch as an independent subscription, and propagating the terminal streaming failure are all solid, and the regression suite is genuinely deterministic; two gaps remain in the reconnect loop itself, plus one visibility nit. Evidence: read of KvClient.java, AbstractClient.java and KvClientTest.java at 3c2f4df; repo greps for streamingCall/listen callers; PD server watch path in KvWatchSubject.notifyClientChangeLeader and KvServiceGrpcImpl.clientWatch; gh -R apache/hugegraph pr checks 3157 (17 pass, only codecov/patch and codecov/project fail).
| } | ||
| try { | ||
| startWatch(subscription); | ||
| } catch (PDException e) { |
There was a problem hiding this comment.
reconnect() catches only PDException, so an unchecked failure ends the retry chain permanently.
reconnect() clears subscription.reconnectScheduled at the top and then calls startWatch guarded only by catch (PDException e). Any RuntimeException escapes into the ScheduledExecutorService, where it is swallowed into the (discarded) ScheduledFuture. At that point the flag is already false, subscription.observer is null, and nothing reschedules — the subscription is left permanently dead, which is the exact failure mode this PR removes.
This is reachable through the production stub path: startWatch → streamingCall → AbstractClient.getStub() → resetStub(). resetStub() assigns leaderHost from the members response (AbstractClient.java:150) before it assigns proxy.setStub(...) (AbstractClient.java:156). If stub creation throws for every host, resetStub() returns a non-empty leaderHost while proxy.getStub() is still null, and getStub() then evaluates setAsyncParams(null, config) → NPE, which is unchecked and outside the try in streamingCall.
No test covers it either: TestKvClient.streamingCall only ever throws PDException (KvClientTest.java:752).
Requested change: catch Throwable (or at least RuntimeException) here and route it through the same scheduleReconnect(subscription) path — or move the rescheduling into a finally — and add a test whose stubbed streamingCall throws an unchecked exception, asserting a further reconnect is still scheduled.
| return false; | ||
| } | ||
|
|
||
| acquire(watchClientId, watchSemaphore); |
There was a problem hiding this comment.
acquire(watchClientId, watchSemaphore) performs an untimed semaphore.acquire() whenever watchClientId == 0 (KvClient.java:340-352). The permit only comes back when some watch receives Starting (KvClient.java:185), fails (KvClient.java:286), or is stopped (KvClient.java:301). Reconnects all run on the single-thread executor created at KvClient.java:85-89.
That combination is exercised by an ordinary leader change: KvWatchSubject.notifyClientChangeLeader() sends Leader_Changed to every observer and then calls removeClient(...), which completes each stream, so all subscriptions call requestReconnect (resetting watchClientId to 0) and queue on that one thread. The first task re-issues its stream and returns; the second parks in acquire until the first stream answers. Since resetStub() builds channels with ManagedChannelBuilder.forTarget(host).usePlaintext() and no keepalive (AbstractClient.java:142), a half-open connection that never delivers Starting and never errors parks that thread for good, and no subscription in the client can reconnect.
Requested change: replace the untimed acquire with tryAcquire(timeout, unit) and treat a timeout as a failed attempt that reschedules, and/or give the reconnect executor more than one thread so a stalled subscription cannot block the others.
| } | ||
| release(watchSemaphore); | ||
| subscriptions.remove(subscription); | ||
| log.error("Watch for key {} stopped after a non-retryable error: {}", |
There was a problem hiding this comment.
🧹 A permanently stopped watch is invisible to the caller.
stopWatch() drops the subscription from subscriptions and records a log line, but the consumer registered through listen/listenPrefix is never told. Both production callers — PdMetaDriver.listen/listenPrefix (hugegraph-server/hugegraph-core/.../meta/PdMetaDriver.java:110-126) and SchemaDriver.listen (hugegraph-struct/.../SchemaDriver.java:199-205) — only see the initial PDException, so after a CANCELLED, UNAUTHENTICATED or PERMISSION_DENIED stream error the application keeps running as if it were still subscribed and the only signal is a log line.
Requested change: give the caller a way to observe termination — for example an optional error/termination callback on listen, or a handle returned from listen whose state can be queried — so a permanently stopped watch is detectable without scraping logs.
There was a problem hiding this comment.
Pull request overview
This PR fixes PD KvClient watch reliability by ensuring watches continue to reconnect after repeated failures, leader changes, and unexpected stream completion, while also addressing a lifecycle bug where transport resets could inadvertently invoke subclass close() behavior.
Changes:
- Refactors
AbstractClientto separate transport cleanup (closeConnections()) from the overridableclose()lifecycle. - Reworks
KvClientwatch handling to track independent watch subscriptions and keep reconnecting with deduped scheduled retries until recovery or client close. - Adds regression tests covering repeated failures/recovery, leader changes, completion recovery, deduplication, stale observers, prefix semantics, and close behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| hugegraph-pd/hg-pd-client/src/main/java/org/apache/hugegraph/pd/client/AbstractClient.java | Avoids invoking subclass lifecycle close() during transport reset; improves streaming-call retry failure signaling. |
| hugegraph-pd/hg-pd-client/src/main/java/org/apache/hugegraph/pd/client/KvClient.java | Introduces per-watch subscription tracking and scheduled reconnect loop to prevent watches permanently stopping. |
| hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/client/KvClientTest.java | Adds deterministic coverage for reconnect behaviors and the transport initialization lifecycle regression. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Purpose of the PR
KvClientpreviously retried a failed watch only once. If that reconnect also failed, thewatch stopped permanently, and unexpected stream completion did not trigger recovery.
The reconnect path also exposed a lifecycle issue:
AbstractClient.resetStub()invoked thevirtual
close()method, so transport initialization on aKvClientdispatched toKvClient.close()and marked the whole client closed.Main Changes
AbstractClienttransport cleanup from the overridable client lifecycle close.onError,Leader_Changed, and unexpectedonCompletedwith a fixed delay until thewatch recovers or the client closes.
KvClient.close()is called.changes, completion, deduplication, stale observers, prefix semantics, and close behavior.
This PR does not add event replay, protocol revisions, PD Server changes, or the Server-side
reconciliation tracked by #3151. Events emitted while a watch is disconnected are still not
replayed.
Verifying these changes
mvn -q -o test -pl hugegraph-pd/hg-pd-test -am -Dtest=KvClientTest -DfailIfNoTests=false -Drat.skip=true -Djacoco.skip=true(12 tests, 0 failures/errors)mvn -q -o package -pl hugegraph-pd -am -DskipTests -Dmaven.javadoc.skip=true -Drat.skip=true -Djacoco.skip=truemvn -q -o -f hugegraph-pd/hg-pd-client/pom.xml apache-rat:checkmvn -q -o -f hugegraph-pd/hg-pd-test/pom.xml apache-rat:checkThe full local PD suite was also attempted. The common suite passed 83/83 and the core suite
passed 89/91 with 2 skipped. The client suite could not complete without a local PD service at
127.0.0.1:8686(4 failures and 5 errors reported connection refused), which prevented the restsuite from executing in that Maven run.
Repository-wide RAT is not a valid signal in this checkout because pre-existing ignored
.upgrade-artifactsfiles produce 68 unrelated unapproved-license entries; RAT passes for bothchanged modules.
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need