feat(pd): add quorum-aware /v1/ready endpoint and raft gauges - #3185
feat(pd): add quorum-aware /v1/ready endpoint and raft gauges#3185bitflicker64 wants to merge 21 commits into
Conversation
/v1/health answers 200 as soon as the Spring listener is up and never consults the raft state, so a PD that has lost its leader keeps reporting healthy to every consumer that gates on it (compose healthchecks, the Store's wait for PD, Kubernetes probes, wait-storage.sh). Keep /v1/health as pure liveness and add an unauthenticated /v1/ready that answers 200 only while the raft node is active and sees a leader, and 503 otherwise. A follower drops its leader id once heartbeats stop inside the election timeout and a leader steps down when it cannot reach a quorum, so "sees a leader" is the local view of being inside a quorum. Export three gauges next to hg_up so operators can alert on quorum loss: hg_raft_leader (1 on the leader), hg_raft_has_leader (1 while a leader is known) and hg_raft_alive_peers (peers the leader heard from inside the election timeout, NaN on non-leaders). Point the compose PD healthchecks at /v1/ready so Stores are no longer released against a leaderless PD, and document both endpoints. The PD startup CI test now also waits for /v1/ready on the live single-node PD, and the REST suite checks the endpoint and the gauges against it. Fixes apache#3183
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3185 +/- ##
=========================================
Coverage 37.78% 37.79%
- Complexity 6560 6563 +3
=========================================
Files 800 800
Lines 68960 68960
Branches 9166 9166
=========================================
+ Hits 26054 26060 +6
+ Misses 39839 39834 -5
+ Partials 3067 3066 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The liveness and readiness split is the right fix for #3183, the jraft assumptions behind it hold, and the new unit tests pass locally. Four minor notes: one on field visibility, two on comment and doc accuracy, one on image-version compatibility for the compose healthcheck change. Evidence: ran mvn -o -pl hugegraph-pd/hg-pd-test -am -Dtest=RaftEngineReadinessTest test on JDK 11 (9/9 pass) after building hg-pd-core and hg-pd-service; checked jraft 1.3.13 directly, where State.isActive() is ordinal() < STATE_ERROR, NodeImpl.listAlivePeers() throws IllegalStateException off-leader under a read lock, and getLeaderId() already maps an empty peer to null; confirmed MetricsConfig.metricsCommonTags adds hg="pd", so the hg_raft_*{ assertions in RestApiTest will match the Prometheus rendering. CI at 4dd7e71 was still running, with the pd, store and hstore integration jobs incomplete, so the live-PD assertions are unverified here.
Make RaftEngine.raftNode volatile so /v1/ready and the hg_raft_* gauges, which read it from request and scrape threads, do not rely on the @PostConstruct ordering for safe publication, and let isReady() reuse the node it already snapshotted instead of re-reading the field. Drop the wait-storage.sh mention from the /v1/ready javadoc: that script polls /v1/stores and Stores register over gRPC, so the compose healthcheck and Kubernetes probes are the real consumers. Move the raft gauge table below the existing /actuator/metrics example so the example still reads as that command's response, and note that both quorum-loss expressions are briefly true during a normal election and need a for: clause longer than the election timeout. State in the docker README that /v1/ready first ships in 1.8.0, since an older HUGEGRAPH_VERSION would leave the PD healthcheck failing and the Stores never starting, and drop a doubled blank line.
…ment health vs ready PD's /v1/health answers 200 as soon as the REST listener is up and never consults raft, so every PD and Store probe and the Store init container's PD wait count listeners, not quorum members (apache#3183). The fix, apache#3185, adds /v1/ready from 1.8.0. - pd.readinessPath and store.waitPath, both defaulting to /v1/health, so the switch to /v1/ready is a values change made with the 1.8.0 pin; the schema rejects paths without a leading slash - README: Limitations entries for the liveness-only health endpoint and for the 45 second discovery lease (measured 30 to 35 seconds); the Store wait is described as a PD wait rather than a quorum wait; the Server now registers its Pod IP, not the Service URL - NOTES and the init container messages no longer claim a quorum - tests: pd_readiness_path_test.yaml, five cases
Points at apache#3185 and says the defaults flip with the 1.8.0 image pin, so the change is not lost once that PR merges.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The endpoint, the raft accessors and the gauges are correct and well covered, and the jraft assumptions behind them hold. Two things to fix before merge: the paragraph added at docker/README.md:210-212 states a failure mode that this PR's own CI disproves, and the compose healthchecks it describes do not actually gate on readiness, because PD answers an unauthenticated request with HTTP 200 and an error body. The pre-PR /v1/health probe had the same property, so this is a missed improvement rather than a regression. Evidence: RestAuthentication.preHandle:61-66 writes the error body and returns false without response.setStatus(...); in CI run 33642694186, job build-server (rocksdb, 11), the hstore smoke pulled Docker Hub hugegraph/pd:latest (git grep '/v1/ready' origin/master -- hugegraph-pd is empty) and logged Container ...-pd-1 Healthy 11 seconds after start; PDCoreSuiteTest (101 run, 2 skipped) and PDRestSuiteTest (16 run) pass at 5bd1b96.
PD's auth interceptor rejects a request by writing an error envelope without setting a status, so every path it does not exclude, including a path that does not exist, answers 200. A healthcheck that only inspects the status code therefore reads a PD too old to carry /v1/ready as ready, which is the same "healthy without a quorum" shape this PR set out to fix. The compose files run published images, so revert their PD healthchecks and the manual verification calls to /v1/health and document what switching them over needs: a body match on "ready":true, and an image that carries the endpoint. Build the /v1/ready body from one RaftEngine.getRaftStatus() snapshot, taken from a single Node reference and a single getLeaderId() read, so a step-down midway cannot report a ready node that knows no leader. Drop the leader's raft address from the body. The endpoint is unauthenticated and the address was the one new disclosure; leadership itself is already published by the hg_raft_leader gauge, and the address stays on the authenticated /v1/members. Call the window in the hg_raft_alive_peers description what jraft measures, the leader lease timeout, which it derives as 90% of the election timeout by default, rather than the election timeout. Assert the empty body in testHealthNeedsNoAuth, since a 200 alone cannot tell an anonymous path from a rejected one.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The readiness implementation is correct, but the deployment guide overstates which compose probe is active. Evidence: JDK 11 RaftEngineReadinessTest passed 10/10; the current compose files still use /v1/health, and the Codecov patch failure is non-blocking.
The startup ordering list said PD healthchecks probe /v1/ready, but c8adc85 put both compose files back on /v1/health and this line was missed, so the guide described a quorum gate that does not exist. Name /v1/health, say it is liveness only, and point at docker/README.md for what pointing the healthchecks at /v1/ready would require.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The liveness and readiness split is the right fix for #3183, and at this head the endpoint, the raft accessors and the three gauges all check out; three minor notes remain, all about wording rather than behaviour. Evidence: jraft-core 1.3.13 State.isActive() is ordinal() < STATE_ERROR.ordinal() over LEADER, TRANSFERRING, CANDIDATE, FOLLOWER, ERROR, ..., so a candidate counts as active; NodeImpl.getAliveNodes compares against leaderLeaseTimeoutMs and calls no checkReplicator, so hg_raft_alive_peers is side-effect free on every scrape; hugegraph-pd/hg-pd-dist/src/assembly/static/conf/application.yml:61 has a single-peer peers-list, so the new wait_for_pd_ready gate in test-start-hugegraph-pd.sh is reachable on a one-node dist; PDService redirects non-leader gRPC calls to the leader (putLicense at PDService.java:1376 is the one exception), so a follower that sees a leader really can serve. CI on ffa13f9 is green except codecov/patch.
State.isActive() is ordinal() < STATE_ERROR over LEADER, TRANSFERRING, CANDIDATE, FOLLOWER, so a candidate is active too and the isReady() javadoc was a state short. Name the set jraft actually uses. Say what the candidate test exercises. jraft clears the leader id before starting an election, so testCandidateWithoutLeaderIsNotReady passes on the missing leader rather than on the state, and a second case records that a candidate does count as active. Mark the empty-peer test as guarding the Node contract, since NodeImpl maps an empty peer to null. Date the interceptor behaviour instead of asserting it as a property of PD. As of 1.7.0 a refusal carries 200 and an error envelope, which is what makes a status-only probe read an older PD as ready, but the body match holds whichever status a refusal carries. Same wording in the docker README and in testHealthNeedsNoAuth. Note that the HEALTHCHECK baked into hugegraph-pd/Dockerfile is on liveness as well. Both compose files override it, so it governs docker run and anything else inheriting the image probe.
RaftEngine.isReady() had no caller outside its own tests: the endpoint reads getRaftStatus() and the gauges read isLeader() and hasLeader(). Remove it and keep its note on the active-state set where isActive() is actually called. testStatusNeverReportsReadyWithoutALeader only repeated the two follower shapes the tests either side of it already cover, so drop it and let the rest assert through the snapshot, which is the path production takes. Reduce wait_for_pd_ready to the gate the docs recommend, curl -f piped into grep. -f rejects the 503 and the body match rejects a 200 that is an auth envelope, so the hand-rolled status parsing bought nothing.
The exclusion list was the one line this change left uncovered, and it carries a contract worth holding: if /v1/ready slips back behind the interceptor, PD answers a probe with 200 and an auth envelope instead of a readiness answer, so every healthcheck matching on the body holds forever while the status still looks healthy. Drive AuthenticationConfigurer with a real InterceptorRegistry and assert through MappedInterceptor.matches(), so the test states the behaviour, that these paths are not intercepted, rather than the literal patterns. /v1/members and friends stay intercepted in the same test. Verified by mutation: dropping /v1/ready from the list fails testProbeEndpointsAreAnonymous.
The pd job runs mvn clean package between the core tests and the codecov upload, which wipes the exec file the core run appended to, so only the client and rest profiles reach the report. Move the check to PDRestSuiteTest, where it also sits closer to the REST layer it covers.
This reverts commit 27f7009. Its reason was wrong: I read the pd job from a stale checkout, where mvn clean package sat between the core tests and the upload. On this branch Package runs first, then the four test profiles append to one exec, and the aggregate report is generated after the rest test, so core-test coverage reaches Codecov either way. With that settled the core suite is the better home. The check is a pure unit test, and the rest profile needs a live PD for the rest of its suite.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The readiness split is well-reasoned and the jraft usage checks out against the pinned jraft-core 1.3.13 (getNodeState()/State.isActive()/listAlivePeers() all behave as the javadoc claims, and getAliveNodes() really does include the node itself); three minor polish items only, none of which should hold up merge. Evidence: git diff 98477f0f5 refs/remotes/pr/3185 for the exact-head diff; javap on com.alipay.sofa.jraft.Node and com.alipay.sofa.jraft.core.State plus NodeImpl.java from the 1.3.13 sources jar (listAlivePeers() throws IllegalStateException off-leader at L2977, getLeaderId() already maps an empty peer to null at L2487, getAliveNodes() adds serverId at L2266); git show 98477f0f5:.../RestAuthentication.java confirms preHandle writes the error envelope without setStatus, so the docs' "match the body, not the status" guidance is correct; MetricsConfig.metricsCommonTags() registers commonTags("hg", "pd"), so the new gauges render with the {...} block the RestApiTest assertions expect; callers of isLeader()/getLeader() audited repo-wide for the new null-safety and none regress. Not verified: no build or test run against this head, and the effective Spring version comes from a parent BOM, so trailing-slash interceptor matching on /v1/ready/ was left out of these comments.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The new interceptor test does not compile at this head, so the PD test and server build jobs cannot pass. Evidence: GitHub Actions run 33839755062 (pd and hstore) and run 33839755249 (both macOS server jobs) report cannot find symbol for AuthenticationConfigurer and RestAuthentication at AuthenticationConfigurerTest.java:53-54; both classes are defined in hg-pd-service.
hg-pd-service is repackaged by spring-boot-maven-plugin, so the artifact a full mvn install leaves behind is an executable jar with classes under BOOT-INF/classes, invisible to javac. A mvn test -am reactor compiles hg-pd-test against target/classes instead, which is why the test passed in the pd job and locally while build-commons and both macOS server jobs failed with cannot-find-symbol. Moving the test into hg-pd-service would not help: every CI package step runs -Dmaven.test.skip=true and the pd profiles only execute suites in hg-pd-test, so it would never run. The anonymity of /v1/health and /v1/ready stays pinned by the live REST tests, which asserted it before the unit test existed.
Derive the snapshot's leader flag from the state read one line above instead of a third locked node call. jraft's isLeader(true) is exactly state == STATE_LEADER, so the value is unchanged while the fields can no longer contradict each other, which is what the javadoc promised. Name the gauges hg.raft.has.leader and hg.raft.alive.peers. Micrometer renders both to the same Prometheus names as before, but dot-separated segments keep other registries from mixing separators. Capture the readiness body in wait_for_pd_ready instead of piping into grep -q: under the script's pipefail a SIGPIPE-killed curl could misread a ready PD, and wait_for_pd already uses the capture shape.
PD gains the quorum-aware /v1/ready endpoint, which answers 503 without a raft leader, and the hg_raft_* gauges. The endpoint sits outside the auth interceptor. The pull request is open against master; this branch carries it so the helm-dev images can be tested with pd.readinessPath and store.waitPath set to /v1/ready (apache#3183).
PD REST now checks the password against auth.secret-key and answers 401 on refusal (apache#3188). The PD image requires HG_PD_AUTH_SECRET_KEY, wait-storage.sh sends PD_AUTH_PASSWORD, and Hubble reads operations.pd.password. Two conflicts with the pull requests merged before it, both resolved as the union: the interceptor exclusion list keeps /v1/ready from apache#3185 alongside the /actuator/** widening from apache#3189, and test-compose.sh runs the startup timeout asserts from apache#3187 followed by the Hubble helper check from apache#3189. render_with_timeout from apache#3187 additionally passes HG_PD_AUTH_SECRET_KEY, which the Compose files require since apache#3189; without it the render step would fail on the two HStore topologies. The chart does not yet supply the PD secret, so PD Pods from an image built at this revision will not start under the chart until that wiring lands.
PD images from 1.8.0 (apache#3189) check the Basic-auth password of every management call against auth.secret-key and refuse to start without one. The chart now keeps that value in a kept release-pd-auth Secret, or in pd.auth.existingSecret, and hands it to the three readers: PD as HG_PD_AUTH_SECRET_KEY, the Server storage wait as PD_AUTH_PASSWORD, and Hubble as operations.pd.password written into its properties file by the existing wrapper. A checksum/pd-auth annotation on the three Pod templates rolls them when the Secret changes; the Server annotations block is now rendered unconditionally for it. Priority and lookup semantics mirror server.auth.token. Older images ignore the password, so the wiring is harmless on the images the draft currently tracks. The values schema requires one of existingSecret, value or autoGenerate and refuses newlines, carriage returns and backslashes in an inline value, since it lands in a Java properties file; the template guard repeats the first rule for values that bypass the schema. The three chart-managed variables join the reserved extraEnv lists. README: Chart Details bullet, four parameter rows, Disaster Recovery calls carry the secret, and the Limitations bullet separates the 1.7.0 behaviour from 1.8.0. NOTES prints how to read the secret. New suite pd_auth_secret_test.yaml, 9 tests; 58 in total. Lint on three presets; renders 16 objects by default and 19 with Hubble. Measured on a kind cluster with images built from master plus apache#3185, apache#3187 and apache#3189: the Secret is created, PD starts with the variable, the Server storage wait passes with the credential, and Hubble lists all nine nodes.
|
Tested end to end on Kubernetes on 2026-09-05, with this branch merged into the hugegraph/hugegraph testing tree. Build under test. Tag What held. Deleting two of three PDs with
One thing worth a look before merge: the handler stalls during the election. With The sample logs and scripts are kept with the campaign notes; I can attach them here if useful. |
|
Logs and scripts from the two runs above, hosted on my fork (branch
The stall, from run 2 ( T0 was 1788593447, the third line's timestamp. The fourth line's request went out at about T0+2.3 s and got its 503 at about T0+12.1 s, 9.79 s later; the gauge on that same line was read after the stall, once the replacements were back, which is why it already shows |
Tested on Kubernetes against three PDs, the first /v1/ready request after two pods were deleted took 9.79 s to return its 503, and with a 2 s client timeout every later sample inside the leaderless window timed out: while jraft runs an election it holds the node lock as it reconnects to peers that no longer answer, and the probe read getNodeState() and getLeaderId() under that lock. Keep a volatile copy of the last announced state and leader visibility in RaftStateMachine, written by onLeaderStart, onLeaderStop, onStartFollowing, onStopFollowing, onError and onShutdown, and serve getRaftStatus(), hasLeader() and the gauges from it. The probe path no longer touches the node at all, which the unit tests now pin with verifyNoInteractions; getAlivePeerCount() checks the lock-free term flag first and only calls listAlivePeers() on a settled leader. jraft emits no callback for candidacy or leadership transfer, so state reports the last announced role and a candidate reads as a follower without a leader, the same not-ready answer as before. Measured with two blackholed peers so every reconnect hangs, the shape of the Kubernetes fault: 15 consecutive /v1/ready samples during the perpetual election all answered 503 in under 21 ms, and a prometheus scrape took 51 ms.
|
Fixed in a3b9395, taking the callback approach from the end-to-end report above. Reproduced the stall shape locally before pushing: a PD electing against two blackholed peers, so every reconnect hangs the way connects to deleted pods do. 15 consecutive Two behaviour notes. |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The callback-served rewrite in a3b9395 is the right answer to the 9.79 s stalled 503, and its callback coverage checks out against the pinned jraft-core 1.3.13: every leader and follower transition PD's raft group can reach emits a callback, and the one place jraft emits none lands on the not-ready side. Two minor items: getRaftStatus() no longer takes the single consistent snapshot its javadoc promises, and STATE_UNINITIALIZED is undocumented although it is what a PD reports for the whole pre-quorum startup window. Evidence: git show a3b939525:<path> for every changed file at the exact head, against merge-base 98477f0; NodeImpl.java and FSMCallerImpl.java from the jraft-core 1.3.13 sources jar pinned at hg-pd-core/pom.xml:41 (onLeaderStart at NodeImpl L2340 and L2926, onLeaderStop from stepDown at L1281 and from transferLeadershipTo at L3246 just after it enters STATE_TRANSFERRING, resetLeaderId firing onStopFollowing at L1198 and onStartFollowing at L1203, and all four delivered asynchronously through the FSM disruptor at FSMCallerImpl L291-320); PDService.java:1706 is the one PD caller of transferLeadershipTo, in updatePdRaft, and it is covered by the onLeaderStop above; PDMetrics.java:49 gives PREFIX = "hg", so the new names render as the hg_raft_* the RestApiTest assertions expect; StoreAPI is @RequestMapping("/v1"), so the new AuthenticationConfigurer exclusion covers the handler; callers of isLeader() and getLeader() audited repo-wide, none relied on the removed NPE; gh pr checks 3185 is green at this head on pd, store, hstore and every build lane, with only codecov/patch red. Not verified: no local build or test run in this session.
getRaftStatus() read probeState and seesLeader as two volatile loads while its javadoc said the fields cannot contradict each other, so a reader landing between onLeaderStart's two stores could see ready:false with state:STATE_LEADER, and the mirror case after onLeaderStop. Hold both values in one immutable ProbeView, written once per callback on the single FSM thread and read once by getRaftStatus() and hasLeader(), the same single-snapshot shape 7074440 gave the node-based version. The javadoc now also says the view trails the node by the FSM queue instead of implying it is current. Document STATE_UNINITIALIZED as what a PD reports until its first raft callback, which is the ordinary startup window before a quorum first forms.
The gauge is NaN on every node but the leader, and a single NaN sample turns sum() or avg() into NaN, so an operator who graphs it across instances gets nothing back. Note that next to the quorum-loss alerts and give the leader-scoped query to use instead.
|
@imbajin Ready for a review pass when you have time. Pushed Why. On the two red codecov marks. What I checked on the branch.
Merge-order conflict with #3189, worth deciding before either lands. Both PRs rewrite the same line. This one makes it excludePathPatterns("/actuator/**", "/v1/health", "/v1/ready", "/v1/prom/targets/*")Dropping Two follow-ups I would not hold the merge for.
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The callback-served readiness path is sound and its jraft assumptions hold up, but getAlivePeerCount() still takes the raft node's read lock, so a metrics scrape can block on the very lock /v1/ready was restructured to avoid. Evidence: jraft-core 1.3.13 sources resolved for hg-pd-core (NodeImpl.listAlivePeers, NodeImpl.stepDown, NodeImpl.preVote, AbstractClientService.connect) read against the head diff.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds a quorum-aware readiness signal for PD and corresponding raft membership gauges, clarifying liveness vs readiness for operators and automation.
Changes:
- Introduces unauthenticated
GET /v1/readythat returns200only when PD sees a raft leader (otherwise503) - Exports raft quorum-related gauges (
hg_raft_leader,hg_raft_has_leader,hg_raft_alive_peers) for alerting - Updates tests, scripts, and docs to use/readiness semantics and avoid false-ready probes
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| hugegraph-store/docs/deployment-guide.md | Documents PD liveness (/v1/health) vs readiness (/v1/ready) for deployment gating |
| hugegraph-server/hugegraph-dist/src/assembly/travis/test-start-hugegraph-pd.sh | Adds readiness wait loop that checks /v1/ready content before proceeding |
| hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/rest/RestApiTest.java | Adds REST tests for anonymous health/ready endpoints and new metrics gauges |
| hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/RaftEngineReadinessTest.java | Adds unit tests for raft-derived readiness behavior and edge cases |
| hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/PDCoreSuiteTest.java | Includes new readiness test in the PD test suite |
| hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/interceptor/AuthenticationConfigurer.java | Excludes /v1/ready from auth interceptor like /v1/health |
| hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/StoreAPI.java | Implements GET /ready endpoint response and status selection |
| hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/metrics/PDMetrics.java | Registers new raft membership gauges |
| hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftStateMachine.java | Adds lock-free probe view updated by raft callbacks |
| hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java | Adds quorum-aware status snapshot, leader visibility helpers, and null-safety |
| hugegraph-pd/docs/api-reference.md | Documents health vs ready endpoints and new metrics |
| hugegraph-pd/README.md | Mentions new liveness/readiness endpoints |
| docker/README.md | Explains why compose healthchecks remain on /v1/health and how to safely switch to readiness |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
listAlivePeers takes the jraft node read lock before it checks for leadership, so a scrape that called it waited out whoever held the write lock. stepDown holds that lock while it writes raft metadata on a term bump, and preVote holds it across a reconnect to every peer, bounded only by raft.rpc-timeout. The gauge now reads a volatile count that a one second refresher publishes on its own daemon thread, and reports -1, so NaN, until the first refresh. jraft walks the same peer set in its own step down timer every half election timeout, so the poll costs nothing next to what the node already does. Drop the javadoc claim that the call only runs on a settled leader, which the lock order cannot support.
Java assert is a no-op unless the jvm runs with -ea, so the health, ready and gauge checks added on this branch could pass without ever running. Convert them, keeping the messages they already carried. The gauge patterns now accept a sample line with no tag block, which micrometer emits for a meter without tags, and anchor on the start of a line so a HELP or TYPE line cannot satisfy them.
The rest suite only reaches the ready path, because the PD it talks to is a single node group that is always its own leader. Nothing pinned the 503 half of the mapping, so an always-200 regression would have shipped. Cover checkReady() over a node that has not started raft and over a follower whose leader went away, plus the leader case so the mapping cannot be inverted either. The test sits in hg-pd-service: the other CI jobs compile hg-pd-test against the repackaged service jar, where the class is not visible.
The probe grepped for the literal "ready":true, so it would stop matching if the serializer ever emitted a space after the colon or pretty-printed the body. Match the key, optional whitespace and the value instead.
scheduleWithFixedDelay cancels every later run if the task throws, and the catch only covered Exception, so an Error would have stopped the refresher for good while the gauge kept serving its last value with no NaN and no log line. Widen it to Throwable, which is what the comment already claimed. shutDown also reset the count before an in-flight refresh could finish, so a refresh already inside listAlivePeers could publish a positive count afterwards. Wait briefly for the executor to quiesce first.
Brings in the master merge already made on the hugegraph/hugegraph mirror, so the fork and the mirror share one history again. Master now carries apache#3187; it does not touch anything this branch changes.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The readiness design holds up against the jraft 1.3.13 sources actually on the classpath — State.isActive() excludes STATE_UNINITIALIZED, NodeImpl.handleElectionTimeout resets the leader id while still STATE_FOLLOWER so onStopFollowing really does fire on heartbeat loss, stepDown fires onLeaderStop before resetting, and all six hooked callbacks are delivered on FSMCallerImpl's single disruptor consumer, so onStopFollowing's read-modify-write is single-threaded as documented. Three minor points only: hasLeader() drops the active-state guard getRaftStatus() applies, the awaitTermination guard in shutDown() cannot prevent what its comment claims, and the PR description no longer matches this head. Evidence: git diff bed2e457..63a26900 in the configured checkout (15 files, +799/-6, matching gh api repos/apache/hugegraph/pulls/3185/files); read com/alipay/sofa/jraft/core/State.java and NodeImpl.java:615-644,1195-1206,1268-1290 from jraft-core-1.3.13-sources.jar; PDConfig.java:153 (raft.rpc-timeout default 10000 ms) against RaftEngine.init's setRpcConnectTimeoutMs; hugegraph-pd/Dockerfile:68-69, docker/docker-compose-hstore.yml:47 and docker/docker-compose-3pd-3store-3server.yml:40 to check the docs' probe claims; install-dist/scripts/dependency/regenerate_known_dependencies.sh (-DincludeScope=runtime) for the new test-scope junit; and the CI job log for this head (run 34155085574, job pd), which shows StoreAPIReadyTest 3/0/0 under surefire (default-test) @ hg-pd-service, PDCoreSuiteTest 102/0/0, PDRestSuiteTest 16/0/0 and PASS PD readiness endpoint reports a raft leader. Not independently built or run locally (only JDK 17 available, project targets 11), and no 3-PD fault injection was reproduced.
| * signal a readiness probe needs. Served from the state machine callbacks, not from the | ||
| * raft node, so it never waits on the node lock. | ||
| */ | ||
| public boolean hasLeader() { |
There was a problem hiding this comment.
🧹 hasLeader() omits the state.isActive() guard that getRaftStatus() applies at L264 (view.state.isActive() && view.seesLeader), yet PDMetrics.registerRaftMeters() documents the gauges as mirroring what GET /v1/ready answers — and hg_raft_has_leader is fed by this method while /v1/ready is fed by getRaftStatus().
The two agree today only by accident of which callbacks write what: the two that set seesLeader = true (onLeaderStart, onStartFollowing) happen to pair it with an active state, and the two that force an inactive state (onError, onShutdown) happen to clear it. ProbeView does not enforce that pairing, so a future callback writing an inactive state without clearing seesLeader would silently split the gauge from the endpoint it is documented to mirror.
Requested change: derive this from the same predicate the endpoint uses, e.g. return getRaftStatus().isReady();, or say in the javadoc why the active-state guard is deliberately omitted here.
| // shutdownNow only interrupts; a refresh already inside | ||
| // listAlivePeers could otherwise publish a positive count | ||
| // after the reset below. | ||
| this.alivePeersRefresher.awaitTermination(1, TimeUnit.SECONDS); |
There was a problem hiding this comment.
🧹 This wait cannot deliver the guarantee the comment above it states. The case it names is a refresh already blocked inside NodeImpl.listAlivePeers(), which parks on ReentrantReadWriteLock.readLock().lock() — not interruptible, so shutdownNow()'s interrupt does not release it. The hold time this whole design exists to avoid is bounded by the raft RPC connect timeout, which init() sets from config.getRpcTimeout() (PDConfig.java:153, default raft.rpc-timeout: 10000), an order of magnitude past the 1 s wait. The false return is discarded too, so shutDown() proceeds to this.alivePeerCount = -1 and the late refresh can still publish a positive count on top of it — exactly the outcome the comment says is prevented.
Impact is small today: shutDown() has no production caller (only PDCoreTestBase:187 and PdTestBase:189), and RaftEngineReadinessTest.setUp() re-runs refreshAlivePeerCount() to clear whatever an earlier class published. But the comment asserts a property the code does not have, and it becomes a real stale-gauge path as soon as shutDown() is wired into a lifecycle hook.
Requested change: make the publish conditional rather than relying on the wait — have refreshAlivePeerCount() write only while a running flag (or a generation token captured at schedule time) is still current — or at minimum check the return value, reset alivePeerCount after it, and log when the refresher did not stop in time.
| * view trails the node by whatever sits in the FSM queue ahead of the announcement, | ||
| * which is the price of never waiting on the node lock. | ||
| */ | ||
| public RaftStatus getRaftStatus() { |
There was a problem hiding this comment.
🧹 The PR description no longer describes this head, and since the repository squash-merges, the body becomes the commit message.
"Main Changes" says RaftEngine gains "hasLeader(), isReady(), getNodeState() and getAlivePeerCount()". At 63a2690 the new public surface is hasLeader(), getRaftStatus() and getAlivePeerCount() plus the nested RaftStatus; there is no RaftEngine.isReady() and no getNodeState() (isReady() lives only on RaftStatus, L290).
"Verify the Changes" lists RaftEngineReadinessTest cases for "empty leader id, candidate, transferring" that the file does not contain — its ten tests are testNotReadyBeforeRaftNodeStarts, testStartedNodeWithoutAnyCallbackIsNotReady, testLeaderIsReady, testFollowerWithLeaderIsReady, testFollowerLosingItsLeaderTurnsNotReady, testLeaderSteppingDownTurnsNotReady, testErrorAndShutdownAreNotReadyEvenAfterLeadership, testProbeNeverTouchesTheRaftNode, testAlivePeerCountSurvivesLeadershipLossRace and testAlivePeerCountOnALeaderAlsoSkipsTheNode — and it never mentions the new StoreAPIReadyTest.
Requested change: refresh both sections against 63a2690 before merge — name getRaftStatus() and the RaftStatus view, drop isReady()/getNodeState(), correct the test-case list, and add StoreAPIReadyTest.
Purpose of the PR
/v1/healthon PD reports healthy without a raft quorum. This adds a quorum-aware readiness signal and leaves/v1/healthas pure liveness.Main Changes
RaftEngine: newhasLeader(),isReady(),getNodeState()andgetAlivePeerCount();isLeader()andgetLeader()are now null-safe before the raft node starts.StoreAPI: new unauthenticatedGET /v1/ready. Returns200with{"ready":true,"state":"STATE_LEADER","isLeader":true}while the raft node is active and sees a leader,503with"ready":falseotherwise. The body comes from oneRaftEngine.getRaftStatus()snapshot and carries no cluster addresses. Added to the auth interceptor exclusion list next to/v1/health.PDMetrics: three gauges for alerting on quorum loss:hg_raft_leader,hg_raft_has_leader,hg_raft_alive_peers(leader only,NaNelsewhere)./v1/health: these files run published images, and PD's auth interceptor answers200on any path it does not exclude, so a status-only probe reads a PD without the endpoint as ready. The docker README records what switching them over needs, a body match on"ready":trueand an image that carries the endpoint.Why "sees a leader" is the right local signal: jraft resets a follower's leader id once heartbeats stop arriving inside the election timeout, and a leader steps down when it cannot reach a quorum. So a non-null leader id means this node is inside a quorum from its own point of view, which is what a readiness probe needs. This matches the behaviour measured in the issue, where the survivor logged
Raft lost leaderwithin a second of the fault.Verify the Changes
RaftEngineReadinessTest(added toPDCoreSuiteTest) covers: no raft node, leader, follower with leader, follower without leader, empty leader id, candidate, transferring, inactive states, and the leadership-loss race ingetAlivePeerCount().RestApiTest(runs against the live CI PD) now checks that/v1/healthanswers200with an empty body, that/v1/readyreportsready=trueandSTATE_LEADERon the single-node PD without disclosing an address, and that the three gauges are exported with the expected values.test-start-hugegraph-pd.shwaits for/v1/readyto return200withready=trueafter the health endpoint responds.pd-rest-test16/16 andtest-start-hugegraph-pd.sh13/13 against a source-built PD. Against a live PD,/v1/readyanswers200{"ready":true,...}as leader and503{"ready":false,...}with two unreachable peers, while/v1/healthstays200throughout and the gauges move1/1/1to0/0/NaN.Does this PR potentially affect the following parts?
Notes for reviewers:
/v1/healthand point readiness probes at/v1/ready. Using/v1/readyas a liveness probe would restart a PD that merely lost its leader./v1/healthis unchanged; this PR only covers PD./v1/readymust match the body, not just the status code.RestAuthentication.preHandlerejects by writing an error envelope without callingsetStatus, so any non-excluded path answers200with{"status":-1,"error":"Unauthorized!"}. Fixing that root cause is out of scope here.Documentation Status
Doc - Updated