Repository navigation
Conversation
A restarted PD can start before the previous JVM has released the metadata RocksDB LOCK, and then fails startup straight away. Retry the open for up to 30 seconds while the failure is a lock conflict, then fail startup as before. Other open failures are not retried. Fixes apache#3226
Each RocksDB open attempt rotates the info LOG, so back off from 500 ms to 4 s instead of retrying every 500 ms, and never sleep past the deadline. Share the open-and-wrap path with snapshot reload, tighten the tests, and correct the docs about how a failed start exits.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3379 +/- ##
============================================
+ Coverage 55.93% 55.95% +0.02%
- Complexity 2277 2322 +45
============================================
Files 774 776 +2
Lines 68534 68898 +364
Branches 9333 9421 +88
============================================
+ Hits 38334 38553 +219
- Misses 26634 26758 +124
- Partials 3566 3587 +21
*This pull request uses carry forward flags. Click here to find out more. ☔ 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 bounded 30 s retry on a held metadata RocksDB LOCK is correct. It retries only lock conflicts, keeps snapshot reload fail-fast, and still fails startup when the wait runs out; I found no actionable issues. Evidence: read the full diff plus HgKVStoreImpl, MetadataFactory, ConfigService, PDService.initService and ShutdownHook.closeOwners, which confirm that the metadata store opens before Raft and that Raft is joined before the metadata store closes; checked against rocksdbjni 8.10.2 that a second process gets IOError 'While lock file' and a same-JVM handle gets 'lock hold by current process'; confirmed the 30 s wait matches SERVER_SHUTDOWN_TIMEOUT_S in stop-hugegraph-pd.sh; CI at 0d0eb70 is green, and the pd job ran PDCoreSuiteTest (193 tests, 0 failures) including the new HgKVStoreImplTest cases.
0d0eb70 to
1c09528
Compare
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Independent cross/adversarial review of this revision: 8/10 readiness; merge candidate, with no new blocking finding in the reviewed change.
Reviewed the backoff, exception/resource handling, zero-wait snapshot overload, tests and documentation. Seven bounded cloud checks using the unchanged extracted open/classification methods with real RocksDBJNI 8.10.2 passed: normal open/read/write; released lock; persistent-lock timeout; snapshot fail-fast; interruption (flag and suppressed cause retained); non-lock failure; and an 8-second held-lock run exercising the capped backoff. These are method-level JNI checks, not a full PD build or service deployment. CI for this head is successful.
The sleep is capped to the remaining retry budget, but native open and scheduling are not preempted: the documented 30-second retry window is not a hard end-to-end startup SLA. Full multi-process restart and same-RC acceptance remain separate evidence gaps, not additional merge prerequisites for this bounded fix. This assessment is newly bound to the linked revision, not inherited from the earlier review.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: An independent PD @PostConstruct path can initialize the JRaft log store before this metadata retry, so a held log LOCK can still abort startup before the retry runs. Evidence: exact-head DiscoveryService, KvServiceGrpcImpl, RaftEngine and JRaft 1.3.14 initialization paths.
| * A restarted PD can start before the previous JVM has released the RocksDB LOCK. | ||
| * Wait about as long as stop-hugegraph-pd.sh waits for that JVM, then fail startup. | ||
| * Every attempt rotates the RocksDB info LOG, so back off between attempts. This | ||
| * covers the metadata store only; PD opens it before the raft log store. |
There was a problem hiding this comment.
DiscoveryService.init() and KvServiceGrpcImpl.init() independently call RaftEngine.init() from @PostConstruct before constructing their metadata-backed services, and no dependency ordering is declared. RaftEngine.init() starts JRaft log storage first; JRaft 1.3.14 throws when that RocksDB open fails, so either callback can abort startup on a still-held pd_raft/log/LOCK before this retry is reached. Please enforce metadata-before-Raft initialization for every startup callback or add the bounded retry to the Raft log store too. Blocking: no. Summary: The new retry can be bypassed by another PD startup path. Evidence: exact-head HugePDServer component scan, DiscoveryService, KvServiceGrpcImpl, RaftEngine, and JRaft 1.3.14 RocksDBLogStorage/RaftServiceFactory paths.
| FileUtils.forceMkdir(new File(dbPath)); | ||
| HgKVStore kvStore = new HgKVStoreImpl(); | ||
| try (Options options = new Options().setCreateIfMissing(true)) { | ||
| RocksDB holder = RocksDB.open(options, dbPath); |
There was a problem hiding this comment.
lock hold by current process diagnostic only. The previous-PD restart path is the separate While lock file diagnostic (RocksDB 8.10.2 env/fs_posix.cc), which the new tests do not exercise. Please add a child-process lock test that verifies initialization waits for release and fails after the configured timeout when the lock remains held. Blocking: no. Summary: The regression tests leave the cross-process lock path uncovered. Evidence: exact-head test line 152; RocksDB 8.10.2 POSIX lock failure returns IOError("While lock file", ...).
DiscoveryService, KvServiceGrpcImpl and PDService each start the shared RaftEngine from @PostConstruct, in no fixed order. Only PDService opened the metadata store first, so a held raft log LOCK could abort startup before the metadata lock wait ran. Start Raft through RaftStartup, which opens metadata first; a stopping PD releases the raft log LOCK before the metadata LOCK. Also test the lock wait against a LOCK held by another process, the restart case, for both wait-then-open and timeout.
hg-pd-service is repackaged as a Spring Boot jar, so hg-pd-test cannot compile against its classes in a packaged build. Keep the package so the service callbacks are unchanged.
There was a problem hiding this comment.
[P2] Clean up native owners when Raft startup fails after metadata opens
At this revision, RaftStartup.init opens/publishes metadata before calling Raft initialization, without failure cleanup. When DiscoveryService or KvServiceGrpcImpl is the first startup callback, a subsequent Raft exception bypasses PDService's existing closeOwners catch. The ShutdownHook depends on successful PDService/KvServiceGrpcImpl construction, so it need not yet exist to close the newly opened metadata owner.
The move to hg-pd-core changes no method content; the startup-owner logic is unchanged. Independent evidence: a controlled actual Spring BeanFactory + RocksDBJNI 8.10.2 lifecycle model (rerun successfully after checking this revision; not a full current-head PD build) produced an unconstructed/uninvoked dependent cleanup hook and a still-held metadata LOCK after destroySingletons when initialization threw after opening metadata. JVM exit releases OS locks; this is not a claim of permanent lock leakage or a reproduced production hang.
Please provide ownership-safe, idempotent failure handling for every startup callback, for example shared initialization/rollback or early owner registration. Preserve shutdown/join ordering before closing metadata, and do not unconditionally close shared owners that another concurrent caller successfully initialized. Add a regression for metadata-open success followed by Raft-init failure. The new order test currently covers metadata-open failure, not this later failure point. The separate-process metadata release/timeout paths independently pass; those checks do not validate full startup ownership. Overall review remains unfinished pending this lifecycle case and current-head CI; no earlier score is inherited.
Update (2026-10-21): the original serial-startup P2 above is addressed in this revision. Raft startup now tracks the metadata store opened by this attempt, joins/shuts down Raft before discarding that store, and preserves an existing or replaced store. The new repository tests cover owned-store release and preserving another owner's store.
Independent bounded checks using the unchanged extracted RaftStartup and MetadataFactory methods, real RocksDBJNI 8.10.2, and a controlled Raft stub passed five cases: owned failure releases the actual LOCK after join; an existing owner remains open without global shutdown; a failed join retains metadata and preserves the original error with suppressed cleanup; an identity-mismatched replacement remains open; and successful startup retains metadata. This is not a full PD build or a general concurrency guarantee. The production entry points reviewed are serial Spring startup callbacks; arbitrary concurrent calls to the public helper remain outside this verification. Current-head CI was still running at verification. This resolves the original finding for that startup path; it does not inherit the earlier overall score or claim same-RC acceptance.
Final readiness update (2026-10-21): independent cross-review of the current revision now rates this bounded fix 8/10; merge candidate. The two child-process lock checks and five serial ownership checks passed, and current-head CI is successful (10 successful checks, 7 conditional skips, no failures). The original P2 is resolved for the reviewed serial startup path. This is a new assessment of this revision, not inherited from the earlier score; arbitrary concurrent helper use, full PD restart and same-RC acceptance remain outside the evidence, not newly imposed merge prerequisites.
When DiscoveryService or KvServiceGrpcImpl starts first and Raft then fails, nothing closed the metadata store RaftStartup had just opened, so its LOCK stayed held until the JVM exited. Close it again after joining Raft, but only when this call opened it; a store opened by PDService or another caller stays with its owner.
3 participants
Purpose of the PR
A restarted PD can start before the previous PD JVM has released the metadata RocksDB
LOCK(systemd restart,stopthenstart). Since #3368 a failed metadata open fails startup instead of leaving a half-started PD, but it fails immediately, so this restart race still needs an operator. #3368 left the retry as a TODO pointing at #3226.Main Changes
HgKVStoreImpl.init()retries the open for up to 30 seconds (the timestop-hugegraph-pd.shwaits for the old JVM) while the failure is a lock conflict, backing off from 500 ms to 4 s because every RocksDB open attempt rotates the infoLOG. After that, startup fails exactly as before (IllegalStateException→PDException→RocksDBException).IOErrorplus RocksDB's lock text:While lock file(another process) orlock hold by current process(this JVM). Both strings are present in rocksdbjni 8.10.2. Other failures, including aLOCKfile that cannot be created, are not retried.hugegraph-pd/docs/configuration.md(PD Core Settings) describes the wait and warns never to deleteLOCKfiles while the old process may be alive;README.mdlinks to it;api-reference.mdno longer calls the retry pending. The two TODOs pointing at [Bug] PD reports "Started" and keeps serving after failing to open its RocksDB (lock held by the previous instance): every request fails, /v1/health stays 200, no retry, no exit #3226 are removed.Known limitation: only the metadata store waits. PD opens it before the raft log store today (
PDService.init), and the shutdown order closes Raft first, so the raft logLOCKis already free by then.Verifying these changes
HgKVStoreImplTestcases: a lock released after 1 s is waited for; a lock never released fails after the (shortened) timeout with the RocksDB cause; a non-lock failure is not retried.mvn test -pl hugegraph-pd/hg-pd-test -am -P pd-core-test: 193 tests, 0 failures, 2 skipped.mvn editorconfig:checkon hg-pd-core, hg-pd-test, hg-pd-service passes.bin/start-hugegraph-pd.sh -d false) while another process heldpd_data/rocksdb/LOCKwithfcntl:While lock file ..., opened the store once it was released, and/v1/healthreturned 200;Failed to open PD metadata, and the process exited with status 1.Does this PR potentially affect the following parts?
LOCKis heldDocumentation Status
Doc - TODO: required documentation is pending; complete it before merging.Doc - Done: documentation is included here or linked below.Doc - No Need: no user-visible documentation is affected.Documentation files in this PR or paired hugegraph-doc PR:
hugegraph-pd/docs/configuration.md,hugegraph-pd/docs/api-reference.md,hugegraph-pd/README.md. No new configuration key; the website PD pages still need to be checked for startup orLOCKguidance before merge.🤖 Generated with Claude Code