imbajin commented on code in PR #3379: URL: https://github.com/apache/hugegraph/pull/3379#discussion_r4344782940
########## hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/service/RaftStartup.java: ########## @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hugegraph.pd.service; + +import org.apache.hugegraph.pd.config.PDConfig; +import org.apache.hugegraph.pd.meta.MetadataFactory; +import org.apache.hugegraph.pd.raft.RaftEngine; +import org.apache.hugegraph.pd.store.HgKVStore; + +/** + * Starts the shared {@link RaftEngine} from any service's startup callback. + */ +public final class RaftStartup { + + private RaftStartup() { + } + + /** + * Open the metadata store before Raft. Its open waits for a LOCK still held by a stopping + * PD, and that PD closes Raft before metadata, so the raft log LOCK is free once the + * metadata store is open. Spring runs the service callbacks in no fixed order, so each of + * them starts Raft through here. If Raft then fails to start, a metadata store this call + * opened is closed again, since a callback other than PDService has no owner to close it. + */ + public static void init(PDConfig pdConfig) { + HgKVStore opened = MetadataFactory.openStore(pdConfig); + try { + RaftEngine.getInstance().init(pdConfig.getRaft()); + } catch (RuntimeException | Error failure) { + // Only a store this call opened is ours to release; PDService and the + // shutdown hook own the rest + if (opened != null) { Review Comment: ⚠️ Important: When `MetadataFactory.openStore()` finds a store already owned by another caller, `opened` is null, but `RaftEngine.init()` still initializes `raftRpcClient` before parsing the peer configuration. If that later step throws, this ownership check skips `RaftEngine.shutDown()` and leaves the initialized RPC client allocated; the added shared-store failure test uses the invalid-peer path. Please stop partial Raft resources for every init failure, and keep metadata discard conditional on ownership. ########## hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/meta/MetadataFactory.java: ########## @@ -56,6 +56,26 @@ public static synchronized HgKVStore getStore(PDConfig pdConfig) { return store; } + /** + * Open the store like {@link #getStore}, and return it only when this call opened it. + */ + public static synchronized HgKVStore openStore(PDConfig pdConfig) { + HgKVStore previous = store; + HgKVStore current = getStore(pdConfig); + return previous == null ? current : null; + } + + /** + * Close a store that a failed startup opened, so its LOCK does not outlive the attempt. + * A store another caller has replaced it with stays open. + */ + public static synchronized void discardStore(HgKVStore opened) { + if (opened != null && store == opened) { + store = null; + opened.close(); Review Comment: ⚠️ Important: This closes the store opened for a failed Raft start, but `RaftKVStore.init()` registered that wrapper in `RaftStateMachine.taskHandlers` before `RaftEngine.init()` ran. `shutDown()` and `resetForNewNode()` retain the handler list, so retrying startup in the same JVM appends a new handler beside the closed one and `onApply()` invokes the old store. Please unregister the handler before closing this store, or rebuild the handler registry after a failed start. ########## hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/store/HgKVStoreImplTest.java: ########## @@ -135,4 +143,159 @@ 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); + // Release the LOCK the way a previous PD JVM does while it is still stopping + Thread releaser = new Thread(() -> { + try { + Thread.sleep(1000L); + } catch (InterruptedException ignored) { + Thread.currentThread().interrupt(); + } + holder.close(); + }, "test-lock-releaser"); + releaser.setDaemon(true); + releaser.start(); + try { + long start = System.nanoTime(); + kvStore.init(config); + long elapsed = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start); + Assert.assertTrue("opened after " + elapsed + " ms", elapsed >= 500L); + byte[] key = "lock".getBytes(StandardCharsets.UTF_8); + kvStore.put(key, key); + Assert.assertArrayEquals(key, kvStore.get(key)); + } finally { + releaser.join(); + kvStore.close(); + FileUtils.deleteQuietly(new File(config.getDataPath())); + } + } + } + + @Test + public void testInitFailsWhenLockIsNeverReleased() throws Exception { + PDConfig config = newConfig("tmp/test-lock-held"); + String dbPath = config.getDataPath() + "/rocksdb"; + FileUtils.forceMkdir(new File(dbPath)); + HgKVStoreImpl kvStore = new HgKVStoreImpl(); + Whitebox.setInternalState(kvStore, "lockWaitTimeoutMs", 1000L); + try (Options options = new Options().setCreateIfMissing(true); + RocksDB ignored = RocksDB.open(options, dbPath)) { + long start = System.nanoTime(); + IllegalStateException e = Assert.assertThrows(IllegalStateException.class, + () -> kvStore.init(config)); + long elapsed = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start); + Assert.assertTrue("gave up after " + elapsed + " ms", elapsed >= 1000L); + Assert.assertTrue(e.getCause() instanceof PDException); + Assert.assertTrue(e.getCause().getCause() instanceof RocksDBException); + } finally { + kvStore.close(); + FileUtils.deleteQuietly(new File(config.getDataPath())); + } + } + + @Test + public void testInitDoesNotRetryOtherOpenFailures() throws Exception { + PDConfig config = newConfig("tmp/test-lock-not-a-dir"); + // A regular file where the RocksDB directory belongs is not a lock conflict + File dbFile = new File(config.getDataPath() + "/rocksdb"); + FileUtils.forceMkdir(dbFile.getParentFile()); + FileUtils.touch(dbFile); + HgKVStore kvStore = new HgKVStoreImpl(); + long start = System.nanoTime(); + try { + IllegalStateException e = Assert.assertThrows(IllegalStateException.class, + () -> kvStore.init(config)); + String message = e.getCause().getCause().getMessage(); + Assert.assertTrue(message, message.contains("is not a directory")); + } finally { + kvStore.close(); + FileUtils.deleteQuietly(new File(config.getDataPath())); + } + long elapsed = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start); + Assert.assertTrue("retried for " + elapsed + " ms", elapsed < 5000L); + } + + @Test(timeout = 60_000L) + public void testInitWaitsForLockHeldByAnotherProcess() throws Exception { + PDConfig config = newConfig("tmp/test-process-lock-released"); + String dbPath = config.getDataPath() + "/rocksdb"; + FileUtils.forceMkdir(new File(dbPath)); + HgKVStore kvStore = new HgKVStoreImpl(); + Process holder = startLockHolder(dbPath, 1000L); Review Comment: ⚠️ Important: `startLockHolder(..., 1000L)` starts its hold before returning the `LOCKED` handshake, while this test only requires `init()` to take 500 ms. A test-thread pause of 500 ms after the handshake can let the helper release the lock before the retry begins, making the test fail even when the retry works. Please release the helper from the test after it observes a retry, or use an explicit synchronization point instead of a fixed hold window. -- 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]
