byteayan commented on code in PR #3379: URL: https://github.com/apache/hugegraph/pull/3379#discussion_r4345663942
########## 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: Fixed in 62e040c. RaftStartup now calls RaftEngine.shutDown() after any init() failure. init() returns early while a node is running, so if it throws, whatever it allocated (here the rpc client) came from this attempt. The metadata store is still discarded only when this call opened it. testRaftFailureKeepsMetadataOpenedByAnotherOwner now also checks that raftRpcClient is null after the failure. ########## 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: Fixed in 62e040c. RaftKVStore.close() removes the store from the state machine's task handlers before closing it, so a retry in the same JVM doesn't leave the closed store registered. That also covers getStore(), where init can fail after the handler is added. The new testRaftFailureUnregistersMetadataHandler enables Raft, fails the startup, and checks that the handler count is unchanged. ########## 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: Fixed in 62e040c. The helper keeps the LOCK until its stdin is closed instead of for a fixed time. The test runs init() asynchronously, asserts it is still waiting after 1.5 s (past the first 500 ms retry), then closes the helper's stdin and waits for init() to finish. -- 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]
