Repository navigation
docs(dist): explain pd and store recovery signals - #532
pallavithegod wants to merge 11 commits into
Conversation
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The new PD health, Store registration and task-response notes match the code in apache/hugegraph#3358 (2aebabd4), #3362 (a63be1c7) and #3359 (277bfea0). Two wording fixes would make the guidance usable: the heartbeat comparison refers to a setting that does not exist, and two links point at issues with several merged PRs instead of the PR that adds the behaviour. Evidence: Read the full diff at 51e8496 and checked each claim against those PR heads: StoreAPI.checkHealthy and RaftStateMachine.onLeaderStop (503 only for STATE_ERROR or STATE_SHUTDOWN), PDRestService.runLeaderTask and API.toJSON(PDException) (NOT_LEADER = 100), TaskAPI patrol and balance bodies, StoreAPI.StoreStatistics.heartbeatAgeMillis, HeartbeatService.registerStore and reportOffline, StoreNodeService.register, and HgStoreEngineOptions.storeHBInterval. The site build workflow at this head is action_required (waiting for maintainer approval), so it has not run.
Specify the fixed Store heartbeat period and link the implementing PRs. Explain keepalive expiry during temporary Offline reporting. Keep reconnect and restore guidance aligned in both languages.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The PD health, Store registration and task response notes match the current heads of apache/hugegraph#3358 (ae49a99e), #3362 (9c08bf6f) and #3359 (277bfea0), and the two earlier wording fixes are in. One sentence in the patrol and balance section describes a response from an endpoint the page never names, and quotes it as a literal whose key order is not stable. Evidence: Read the full diff at d28cbfb and both language pages around it. Checked each claim against the code PR heads: StoreAPI.checkHealthy with RaftStateMachine.onError, onShutdown and onLeaderStop (503 only for STATE_ERROR or STATE_SHUTDOWN); the keepalive change in StoreNodeService.updateStore and register at 9c08bf6f; HeartbeatService.registerStore (restore once per process) and reportOffline; StoreAPI.StoreStatistics.heartbeatAgeMillis; HgStoreEngineOptions.storeHBInterval (fixed 30 s); PDRestService.runLeaderTask with NOT_LEADER = 100; TaskAPI patrol, balancePartitions and balanceLeaders bodies; TaskScheduleService.balancePartitionLeader gates. Ran Map.of("status", 0, "outcome", "skipped") in six fresh JVMs: two printed status first, four printed outcome first. The site build at this head is action_required (waiting for maintainer approval), so it has not run.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Two new recovery notes need clarification around the single-PD Helm probe default and heartbeat freshness after PD clock changes. Evidence: checked the linked implementation at apache/hugegraph#3358 and #3362 exact heads.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Score: 8.5/10 (10 - 0.5 x 3 Minor). Summary: The PD health, probe, Store registration, heartbeat age and task response notes match the current heads of apache/hugegraph#3358 (ae49a99e6), #3362 (9c08bf6fb) and #3359 (277bfea04), and EN and CN agree. Three sentences still state behaviour that open threads on #3358 and #3362 contradict. Because this page links those still-open PRs as fixes, it should merge after #3358, #3359 and #3362, and the three sentences should be rechecked against what lands. Evidence: read the full diff at 5a1f9c6 and the delta since d28cbfb, and checked it against StoreAPI.checkHealthy, the hugegraph.pd.livenessPath helper and pd-statefulset.yaml (startup and liveness share the path; pd.livenessPath overrides both), StoreNodeService.register, HeartbeatService.reportOffline, PDService.setStore, StoreAPI.StoreStatistics.heartbeatAgeMillis (max(0, now - lastHeartBeat), -1 when unknown) and HgStoreEngineOptions.storeHBInterval (fixed 30). git diff --check is clean. The site build at this head is action_required, so it has not run.
Earlier comments, verified at this head:
- Heartbeat period wording (mine): fixed in d28cbfb.
- Links to #3359 and #3362 instead of issues (mine): fixed in d28cbfb.
- Skipped outcome tied to
/v1/task/balancePartitionsand no literal key order (mine): fixed in 5a1f9c6. - Single-PD probe default and
pd.livenessPathoverride (@imbajin): fixed in 5a1f9c6. heartbeatAgeMilliswall-clock caveat (@imbajin): fixed in 5a1f9c6.
Explain renewal during recovery and expiry after shutdown. Document safe fallback when older PDs reject the shutdown RPC. Keep the English and Chinese operational guidance aligned.
Preserve the maintainer review corrections and updated master documentation. Combine the Store lease and shutdown compatibility guidance. Keep both language pages aligned.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The restore gate, shutdown report, lease renewal, heartbeatAgeMillis and task response notes match the current heads of apache/hugegraph#3358 (e7858813), #3359 (f69a7b93) and #3362 (5511593e), and the three Minor threads from the 5a1f9c6 round are resolved there. One sentence frames the single-PD pd.livenessPath=/v1/health override as enabling recovery that the default /v1/ready probe already provides, while dropping the restart for a PD that never initialises (inline). Evidence: read the full diff against base 278fdcb and the 5a1f9c6..a7a1697 delta. For the finding: StoreAPI.checkHealthy, RaftEngine.getRaftStatus, the hugegraph.pd.livenessPath helper, pd-statefulset.yaml and the startup budget in values.yaml at #3358 head, and the repro in apache/hugegraph#3226. The other claims were checked against StoreNodeService.register / reportStoreShutdown, HeartbeatService.registerStore / reportOffline, StoreAPI.StoreStatistics at #3362 head and PDRestService.runLeaderTask / TaskAPI at #3359 head. dist/validate-links.sh and git diff --check pass. A local Hugo build did not finish (the site needs Hugo 0.168.0, 0.166.0 is installed), and the site build workflow at this head is action_required.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Two new Important documentation gaps remain in PD restart behavior and the Store shutdown signal. Evidence: verified against the exact #532 head, the linked #3358 Helm probe defaults and #3362 shutdown RPC handling.
Explain automatic single-PD and multi-PD restarts. Qualify shutdown reports with credential and expiry behavior. Keep placement eligibility and restoration retry guidance aligned.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The PD health, probe-path, Store registration, shutdown report, lease, heartbeatAgeMillis and task response notes match apache/hugegraph#3358 as merged (fd115938) and the current heads of #3362 (ebdc8386) and #3359 (14a91a49), EN and CN agree, and every earlier thread is addressed at this head. One sentence tells Helm users to set HG_PD_AUTH_SECRET_KEY on Store by hand, although #3362's chart already wires it from the PD auth Secret (inline). Evidence: read the full diff against merge base df23ce2 and both pages around it. Checked StoreAPI.checkHealthy, RaftStateMachine terminal guards and RaftEngine.getRaftStatus at master, the hugegraph.pd.livenessPath helper and pd-statefulset.yaml probes; StoreNodeService.register/reportStoreShutdown/getPlacementStores, HeartbeatService.registerStore/reportOffline (5 s Offline renewal cap, restore backoff to the heartbeat interval), StoreShutdownAuthentication, DefaultPdProvider.authority, StoreAPI.StoreStatistics.heartbeatAgeMillis and store-statefulset.yaml at #3362 head; PDRestService.runLeaderTask (NOT_LEADER = 100), TaskAPI.patrolPartitions/balancePartitions/balanceLeaders and TaskScheduleService.patrolPartitionsWithResult at #3359 head. dist/validate-links.sh and git diff --check pass. The site build at this head is action_required, so it has not run.
| and attempt to report `Offline` on graceful shutdown; PD changes its record only if it accepts the report. Upgrade PD before Store to enable the restore gate. | ||
| Shutdown reports preserve the existing keepalive window without renewing it; expiry still applies if the Store does not return. | ||
| During recovery, Offline registrations renew the returned Store's lease while its engines restore on an owned worker. | ||
| The shutdown RPC requires a privileged internal credential: set `HG_PD_AUTH_SECRET_KEY` in Store to match PD's `auth.secret-key`. |
There was a problem hiding this comment.
Minor: On this Helm page, "set HG_PD_AUTH_SECRET_KEY in Store" reads as a manual step, but the chart in apache/hugegraph#3362 (head ebdc8386) already does it. store-statefulset.yaml adds HG_PD_AUTH_SECRET_KEY from secretKeyRef hugegraph.pd.authSecretName / hugegraph.pd.authSecretKey, the same Secret and key that pd-statefulset.yaml gives PD, and credential_contract_test.yaml asserts it for pd.auth.existingSecret and pd.auth.key. The only way to follow this sentence with the chart is store.extraEnv, which renders after the chart's entry, so a mismatched value silently replaces the chart's Secret reference and the shutdown report is refused (Store then falls back to lease expiry). The open review on #3362 also asks for this name to become reserved in store.extraEnv, which would make the step fail to render.
Please say that the chart passes PD's auth Secret (pd.auth.existingSecret / pd.auth.key) to Store automatically, and keep the manual HG_PD_AUTH_SECRET_KEY instruction only for deployments outside this chart or chart versions without #3362. Make the same change to line 137 of the Chinese page.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Three recovery statements need qualification around old PD images, shared shutdown credential scope and an ambiguous RPC result. Evidence: apache/hugegraph#3358 API-reference behavior and #3362 StoreShutdownAuthentication, PDService and StoreNodeService paths.
| after a terminal Raft error or shutdown; ordinary elections and quorum loss remain live (`200`). | ||
| After a failed snapshot, repair the disk or storage problem before an operator-initiated restart; freeing space alone does not reinitialize JRaft. | ||
| The probes below may already restart a failed PD automatically. If the underlying storage fault remains, those restarts can repeat without fixing it. | ||
| When `pd.replicas` is 1, chart templates default startup and liveness probes to `/v1/ready`, which already restarts a single PD after a terminal Raft error. |
There was a problem hiding this comment.
/v1/ready do not serve that endpoint; an unknown-path request can return HTTP 200 with an Unauthorized body, which satisfies Kubernetes' status-only httpGet probe and will not restart a terminal PD. Please limit this sentence to images that include the endpoint and identify the supported image boundary. Evidence: The merged #3358 API-reference note documents the unknown-path 200 response and warns that a status-only probe can treat it as ready.
| and attempt to report `Offline` on graceful shutdown; PD changes its record only if it accepts the report. Upgrade PD before Store to enable the restore gate. | ||
| Shutdown reports preserve the existing keepalive window without renewing it; expiry still applies if the Store does not return. | ||
| During recovery, Offline registrations renew the returned Store's lease while its engines restore on an owned worker. | ||
| The shutdown RPC requires a privileged internal credential: set `HG_PD_AUTH_SECRET_KEY` in Store to match PD's `auth.secret-key`. |
There was a problem hiding this comment.
ReportStoreShutdown with another Store ID. Please state that scope and restrict secret access to trusted internal Store workloads. Evidence: #3362 StoreShutdownAuthentication gates on the shared credential, while PDService.reportStoreShutdown forwards the request's Store ID directly to StoreNodeService.reportStoreShutdown.
| Shutdown reports preserve the existing keepalive window without renewing it; expiry still applies if the Store does not return. | ||
| During recovery, Offline registrations renew the returned Store's lease while its engines restore on an owned worker. | ||
| The shutdown RPC requires a privileged internal credential: set `HG_PD_AUTH_SECRET_KEY` in Store to match PD's `auth.secret-key`. | ||
| If the report is refused or fails, Store still shuts down and PD retains its previous record until the existing lease expires. |
There was a problem hiding this comment.
PDService calls StoreNodeService.reportStoreShutdown before sending the response, and that method persists the Offline state before returning.
Main Changes (主要改动)
Pair with apache/hugegraph#3358, apache/hugegraph#3359 and apache/hugegraph#3362.
Verifying these changes (验证改动)
bash dist/validate-links.shgit diff --checkCoordinate this prose update with the linked code PRs; the existing older-image workarounds remain documented.