imbajin commented on code in PR #3379:
URL: https://github.com/apache/hugegraph/pull/3379#discussion_r4334969494
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/store/HgKVStoreImplTest.java:
##########
@@ -135,4 +141,93 @@ public void TestSnapshot() throws PDException {
kvStore.close();
}
+
+ @Test
+ public void testInitWaitsForHeldLock() throws Exception {
+ PDConfig config = newConfig("tmp/test-lock-released");
+ String dbPath = config.getDataPath() + "/rocksdb";
+ FileUtils.forceMkdir(new File(dbPath));
+ HgKVStore kvStore = new HgKVStoreImpl();
+ try (Options options = new Options().setCreateIfMissing(true)) {
+ RocksDB holder = RocksDB.open(options, dbPath);
Review Comment:
⚠️ Important: This new test opens its lock holder in the same JUnit JVM, so
it exercises RocksDB's `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", ...)`.
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/store/HgKVStoreImpl.java:
##########
@@ -55,10 +56,21 @@ public class HgKVStoreImpl implements HgKVStore {
private static final ConcurrentHashMap<String,
ConcurrentMap<String, Object>> CACHE = new ConcurrentHashMap();
+ /*
+ * 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.
Review Comment:
⚠️ Important: The metadata-before-Raft order is not guaranteed:
`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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
