From c4e7e42771f2c2382f6bee3633a94efcbc7d0d3f Mon Sep 17 00:00:00 2001 From: jonmv Date: Thu, 4 Apr 2024 12:37:15 +0200 Subject: [PATCH 1/8] Avoid NPE when closing SendAckRequestProcessor after other failure --- .../zookeeper/server/quorum/Learner.java | 28 +++++++++---------- .../quorum/SendAckRequestProcessor.java | 6 ++-- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java index 3fe981ccef6..7b681ec5717 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java @@ -900,26 +900,26 @@ boolean isRunning() { } void closeSocket() { - if (sock != null) { - if (sockBeingClosed.compareAndSet(false, true)) { - if (closeSocketAsync) { - final Thread closingThread = new Thread(() -> closeSockSync(), "CloseSocketThread(sid:" + zk.getServerId()); - closingThread.setDaemon(true); - closingThread.start(); - } else { - closeSockSync(); - } + if (sockBeingClosed.compareAndSet(false, true)) { + if (sock == null) { // Closing before establishing the connection is a noop + return; + } + Socket socket = sock; + sock = null; + if (closeSocketAsync) { + final Thread closingThread = new Thread(() -> closeSockSync(socket), "CloseSocketThread(sid:" + zk.getServerId()); + closingThread.setDaemon(true); + closingThread.start(); + } else { + closeSockSync(socket); } } } - void closeSockSync() { + private static void closeSockSync(Socket socket) { try { long startTime = Time.currentElapsedTime(); - if (sock != null) { - sock.close(); - sock = null; - } + socket.close(); ServerMetrics.getMetrics().SOCKET_CLOSING_TIME.add(Time.currentElapsedTime() - startTime); } catch (IOException e) { LOG.warn("Ignoring error closing connection to leader", e); diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java index 78c9dbd39f7..868fb2b4627 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java @@ -20,6 +20,8 @@ import java.io.Flushable; import java.io.IOException; +import java.net.Socket; + import org.apache.zookeeper.ZooDefs.OpCode; import org.apache.zookeeper.server.Request; import org.apache.zookeeper.server.RequestProcessor; @@ -46,7 +48,7 @@ public void processRequest(Request si) { learner.writePacket(qp, false); } catch (IOException e) { LOG.warn("Closing connection to leader, exception during packet send", e); - learner.closeSockSync(); + learner.closeSocket(); } } } @@ -56,7 +58,7 @@ public void flush() throws IOException { learner.writePacket(null, true); } catch (IOException e) { LOG.warn("Closing connection to leader, exception during packet send", e); - learner.closeSockSync(); + learner.closeSocket(); } } From 8d055cca63e7f64dfaab14a4804bb5f096722bf7 Mon Sep 17 00:00:00 2001 From: jonmv Date: Thu, 4 Apr 2024 12:45:17 +0200 Subject: [PATCH 2/8] Fix shutdown method overrides, so, e.g., SyncRequestProcessor is shut down from Learner --- .../java/org/apache/zookeeper/server/ZooKeeperServer.java | 2 +- .../apache/zookeeper/server/quorum/LeaderZooKeeperServer.java | 4 ++-- .../zookeeper/server/quorum/LearnerZooKeeperServer.java | 4 ++-- .../zookeeper/server/quorum/ObserverZooKeeperServer.java | 4 ++-- .../zookeeper/server/quorum/ReadOnlyZooKeeperServer.java | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java index 598a6976876..093c695b82f 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java @@ -924,7 +924,7 @@ public boolean isRunning() { return state == State.RUNNING; } - public void shutdown() { + public final void shutdown() { shutdown(false); } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java index 2de080bd380..1f629bed73d 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java @@ -155,11 +155,11 @@ protected void unregisterMetrics() { } @Override - public synchronized void shutdown() { + public synchronized void shutdown(boolean fullyShutDown) { if (containerManager != null) { containerManager.stop(); } - super.shutdown(); + super.shutdown(fullyShutDown); } @Override diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java index efd2e376208..572ae43aba8 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java @@ -152,14 +152,14 @@ protected void unregisterJMX(Learner peer) { } @Override - public synchronized void shutdown() { + public synchronized void shutdown(boolean fullyShutDown) { if (!canShutdown()) { LOG.debug("ZooKeeper server is not running, so not proceeding to shutdown!"); return; } LOG.info("Shutting down"); try { - super.shutdown(); + super.shutdown(fullyShutDown); } catch (Exception e) { LOG.warn("Ignoring unexpected exception during shutdown", e); } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java index 79aa10ca9de..44a410f5e61 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java @@ -128,12 +128,12 @@ public String getState() { } @Override - public synchronized void shutdown() { + public synchronized void shutdown(boolean fullyShutDown) { if (!canShutdown()) { LOG.debug("ZooKeeper server is not running, so not proceeding to shutdown!"); return; } - super.shutdown(); + super.shutdown(fullyShutDown); if (syncRequestProcessorEnabled && syncProcessor != null) { syncProcessor.shutdown(); } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java index 0efa4677053..57c007c4c7d 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java @@ -190,7 +190,7 @@ public long getServerId() { } @Override - public synchronized void shutdown() { + public synchronized void shutdown(boolean fullyShutDown) { if (!canShutdown()) { LOG.debug("ZooKeeper server is not running, so not proceeding to shutdown!"); return; @@ -206,7 +206,7 @@ public synchronized void shutdown() { self.adminServer.setZooKeeperServer(null); // shutdown the server itself - super.shutdown(); + super.shutdown(fullyShutDown); } @Override From d82213ee8ef019fbe8ef18d7c97e6ff2423f6b76 Mon Sep 17 00:00:00 2001 From: jonmv Date: Thu, 4 Apr 2024 12:51:11 +0200 Subject: [PATCH 3/8] Remove shutdown(boolean) from OZKS, which was identical to that of LZKS --- .../server/quorum/ObserverZooKeeperServer.java | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java index 44a410f5e61..74663a9c136 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java @@ -44,7 +44,7 @@ public class ObserverZooKeeperServer extends LearnerZooKeeperServer { * take periodic snapshot. Default is ON. */ - private boolean syncRequestProcessorEnabled = this.self.getSyncEnabled(); + private final boolean syncRequestProcessorEnabled = this.self.getSyncEnabled(); /* * Pending sync requests @@ -127,18 +127,6 @@ public String getState() { return "observer"; } - @Override - public synchronized void shutdown(boolean fullyShutDown) { - if (!canShutdown()) { - LOG.debug("ZooKeeper server is not running, so not proceeding to shutdown!"); - return; - } - super.shutdown(fullyShutDown); - if (syncRequestProcessorEnabled && syncProcessor != null) { - syncProcessor.shutdown(); - } - } - @Override public void dumpMonitorValues(BiConsumer response) { super.dumpMonitorValues(response); From 13001bf30d0a778582dbdcca946f7ec36d71eb2c Mon Sep 17 00:00:00 2001 From: jonmv Date: Thu, 4 Apr 2024 12:59:26 +0200 Subject: [PATCH 4/8] Correctly propagate ZKDB.clear during shutdown of not-running ZKS children --- .../apache/zookeeper/server/quorum/LearnerZooKeeperServer.java | 1 + .../apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java | 1 + 2 files changed, 2 insertions(+) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java index 572ae43aba8..92ddaacdec9 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java @@ -154,6 +154,7 @@ protected void unregisterJMX(Learner peer) { @Override public synchronized void shutdown(boolean fullyShutDown) { if (!canShutdown()) { + super.shutdown(fullyShutDown); LOG.debug("ZooKeeper server is not running, so not proceeding to shutdown!"); return; } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java index 57c007c4c7d..112654a7a07 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java @@ -192,6 +192,7 @@ public long getServerId() { @Override public synchronized void shutdown(boolean fullyShutDown) { if (!canShutdown()) { + super.shutdown(fullyShutDown); LOG.debug("ZooKeeper server is not running, so not proceeding to shutdown!"); return; } From 5d7aa33f0274b47ca5771aa6ea6f79998cbdf79d Mon Sep 17 00:00:00 2001 From: jonmv Date: Thu, 4 Apr 2024 13:01:30 +0200 Subject: [PATCH 5/8] Shut down sync processor before parent ZKS; it uses TXN-log during partial shutdown --- .../server/quorum/LearnerZooKeeperServer.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java index 92ddaacdec9..a6de485b1b1 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java @@ -159,11 +159,6 @@ public synchronized void shutdown(boolean fullyShutDown) { return; } LOG.info("Shutting down"); - try { - super.shutdown(fullyShutDown); - } catch (Exception e) { - LOG.warn("Ignoring unexpected exception during shutdown", e); - } try { if (syncProcessor != null) { syncProcessor.shutdown(); @@ -171,6 +166,11 @@ public synchronized void shutdown(boolean fullyShutDown) { } catch (Exception e) { LOG.warn("Ignoring unexpected exception in syncprocessor shutdown", e); } + try { + super.shutdown(fullyShutDown); + } catch (Exception e) { + LOG.warn("Ignoring unexpected exception during shutdown", e); + } } } From 41e6e955e840fe2318e565223dadbb49c3aacfd7 Mon Sep 17 00:00:00 2001 From: Kezhu Wang Date: Thu, 19 Sep 2024 22:01:14 +0800 Subject: [PATCH 6/8] ZOOKEEPER-4712: Add test case to assert request processors got shutdown in ZooKeeperServer::shutdown --- .../quorum/SendAckRequestProcessor.java | 2 - .../server/ZooKeeperServerShutdownTest.java | 85 +++++++++++++++++++ 2 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerShutdownTest.java diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java index 868fb2b4627..3f42c2bd30f 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java @@ -20,8 +20,6 @@ import java.io.Flushable; import java.io.IOException; -import java.net.Socket; - import org.apache.zookeeper.ZooDefs.OpCode; import org.apache.zookeeper.server.Request; import org.apache.zookeeper.server.RequestProcessor; diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerShutdownTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerShutdownTest.java new file mode 100644 index 00000000000..b0dd19cb77c --- /dev/null +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerShutdownTest.java @@ -0,0 +1,85 @@ +/* + * 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.zookeeper.server; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import java.io.File; +import java.io.IOException; +import org.apache.zookeeper.ZKTestCase; +import org.apache.zookeeper.server.persistence.FileTxnSnapLog; +import org.apache.zookeeper.server.quorum.Learner; +import org.apache.zookeeper.server.quorum.LearnerZooKeeperServer; +import org.apache.zookeeper.server.quorum.QuorumPeer; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +public class ZooKeeperServerShutdownTest extends ZKTestCase { + + static class ShutdownTrackRequestProcessor implements RequestProcessor { + boolean shutdown = false; + + @Override + public void processRequest(Request request) throws RequestProcessorException { + } + + @Override + public void shutdown() { + shutdown = true; + } + } + + public static class ShutdownTrackLearnerZooKeeperServer extends LearnerZooKeeperServer { + public ShutdownTrackLearnerZooKeeperServer(FileTxnSnapLog logFactory, QuorumPeer self) throws IOException { + super(logFactory, 2000, 2000, 2000, -1, new ZKDatabase(logFactory), self); + } + + @Override + protected void setupRequestProcessors() { + firstProcessor = new ShutdownTrackRequestProcessor(); + syncProcessor = new SyncRequestProcessor(this, null); + syncProcessor.start(); + } + + ShutdownTrackRequestProcessor getFirstProcessor() { + return (ShutdownTrackRequestProcessor) firstProcessor; + } + + SyncRequestProcessor getSyncRequestProcessor() { + return syncProcessor; + } + + @Override + public Learner getLearner() { + return null; + } + } + + @Test + void testLearnerZooKeeperServerShutdown(@TempDir File tmpDir) throws Exception { + File tmpFile = File.createTempFile("test", ".dir", tmpDir); + tmpFile.delete(); + FileTxnSnapLog logFactory = new FileTxnSnapLog(tmpFile, tmpFile); + ShutdownTrackLearnerZooKeeperServer zooKeeperServer = new ShutdownTrackLearnerZooKeeperServer(logFactory, new QuorumPeer()); + zooKeeperServer.startup(); + zooKeeperServer.shutdown(false); + assertTrue(zooKeeperServer.getFirstProcessor().shutdown); + assertFalse(zooKeeperServer.getSyncRequestProcessor().isAlive()); + } +} From c0682d75ee9c3a041aa4ce1bd26a70f83abdb8e4 Mon Sep 17 00:00:00 2001 From: jonmv Date: Fri, 20 Sep 2024 10:21:47 +0200 Subject: [PATCH 7/8] Refactor shutdown to avoid requiring repeated logic in children --- .../apache/zookeeper/server/ZKDatabase.java | 2 +- .../zookeeper/server/ZooKeeperServer.java | 68 ++++++++++--------- .../zookeeper/server/ZooKeeperServerMain.java | 4 +- .../server/persistence/FileTxnSnapLog.java | 2 +- .../server/quorum/LeaderZooKeeperServer.java | 4 +- .../server/quorum/LearnerZooKeeperServer.java | 10 +-- .../quorum/ReadOnlyZooKeeperServer.java | 9 +-- .../server/quorum/WatchLeakTest.java | 2 +- 8 files changed, 45 insertions(+), 56 deletions(-) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java index c1e0b4834a3..c27ac6aa83b 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java @@ -294,7 +294,7 @@ public long loadDataBase() throws IOException { } /** - * Fast forward the database adding transactions from the committed log into memory. + * Fast-forward the database adding transactions from the committed log into memory. * @return the last valid zxid. * @throws IOException */ diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java index 093c695b82f..dc446645e10 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java @@ -913,7 +913,7 @@ protected void setState(State state) { * @return true if the server is running or server hits an error, false * otherwise. */ - protected boolean canShutdown() { + private boolean canShutdown() { return state == State.RUNNING || state == State.ERROR; } @@ -930,21 +930,43 @@ public final void shutdown() { /** * Shut down the server instance - * @param fullyShutDown true if another server using the same database will not replace this one in the same process + * @param fullyShutDown true when no other server will use the same database to replace this one */ - public synchronized void shutdown(boolean fullyShutDown) { - if (!canShutdown()) { - if (fullyShutDown && zkDb != null) { - zkDb.clear(); + public final synchronized void shutdown(boolean fullyShutDown) { + if (canShutdown()) { + LOG.info("Shutting down"); + + shutdownComponents(); + + if (zkDb != null && !fullyShutDown) { + // There is no need to clear the database if we are going to reuse it: + // * When a new quorum is established we can still apply the diff + // on top of the same zkDb data + // * If we fetch a new snapshot from leader, the zkDb will be + // cleared anyway before loading the snapshot + try { + // This will fast-forward the database to the last recorded transaction + zkDb.fastForwardDataBase(); + } catch (IOException e) { + LOG.error("Error updating DB", e); + fullyShutDown = true; + } } + setState(State.SHUTDOWN); + } else { LOG.debug("ZooKeeper server is not running, so not proceeding to shutdown!"); - return; } - LOG.info("shutting down"); - - // new RuntimeException("Calling shutdown").printStackTrace(); - setState(State.SHUTDOWN); + if (zkDb != null && fullyShutDown) { + zkDb.clear(); + } + } + /** + * @implNote + * Shuts down components owned by this class; + * remember to call super.shutdownComponents() when overriding! + */ + protected void shutdownComponents() { // unregister all metrics that are keeping a strong reference to this object // subclasses will do their specific clean up unregisterMetrics(); @@ -953,9 +975,8 @@ public synchronized void shutdown(boolean fullyShutDown) { requestThrottler.shutdown(); } - // Since sessionTracker and syncThreads poll we just have to - // set running to false and they will detect it during the poll - // interval. + // Since sessionTracker and syncThreads poll we just have to set running to false, + // and they will detect it during the poll interval. if (sessionTracker != null) { sessionTracker.shutdown(); } @@ -966,25 +987,6 @@ public synchronized void shutdown(boolean fullyShutDown) { jvmPauseMonitor.serviceStop(); } - if (zkDb != null) { - if (fullyShutDown) { - zkDb.clear(); - } else { - // else there is no need to clear the database - // * When a new quorum is established we can still apply the diff - // on top of the same zkDb data - // * If we fetch a new snapshot from leader, the zkDb will be - // cleared anyway before loading the snapshot - try { - //This will fast forward the database to the latest recorded transactions - zkDb.fastForwardDataBase(); - } catch (IOException e) { - LOG.error("Error updating DB", e); - zkDb.clear(); - } - } - } - requestPathMetricsCollector.shutdown(); unregisterJMX(); } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java index d6cbbe4a1ee..24e7f5b180e 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java @@ -192,9 +192,7 @@ public void runFromConfig(ServerConfig config) throws IOException, AdminServerEx if (secureCnxnFactory != null) { secureCnxnFactory.join(); } - if (zkServer.canShutdown()) { - zkServer.shutdown(true); - } + zkServer.shutdown(true); } catch (InterruptedException e) { // warn, but generally this is ok LOG.warn("Server interrupted", e); diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java index 29b4c0a616d..2816826046e 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java @@ -313,7 +313,7 @@ public long restore(DataTree dt, Map sessions, PlayBackListener l } /** - * This function will fast forward the server database to have the latest + * This function will fast-forward the server database to have the latest * transactions in it. This is the same as restore, but only reads from * the transaction logs and not restores from a snapshot. * @param dt the datatree to write transactions to. diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java index 1f629bed73d..ef9b0737268 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java @@ -155,11 +155,11 @@ protected void unregisterMetrics() { } @Override - public synchronized void shutdown(boolean fullyShutDown) { + protected void shutdownComponents() { if (containerManager != null) { containerManager.stop(); } - super.shutdown(fullyShutDown); + super.shutdownComponents(); } @Override diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java index a6de485b1b1..a7c345d0757 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java @@ -152,13 +152,7 @@ protected void unregisterJMX(Learner peer) { } @Override - public synchronized void shutdown(boolean fullyShutDown) { - if (!canShutdown()) { - super.shutdown(fullyShutDown); - LOG.debug("ZooKeeper server is not running, so not proceeding to shutdown!"); - return; - } - LOG.info("Shutting down"); + protected void shutdownComponents() { try { if (syncProcessor != null) { syncProcessor.shutdown(); @@ -167,7 +161,7 @@ public synchronized void shutdown(boolean fullyShutDown) { LOG.warn("Ignoring unexpected exception in syncprocessor shutdown", e); } try { - super.shutdown(fullyShutDown); + super.shutdownComponents(); } catch (Exception e) { LOG.warn("Ignoring unexpected exception during shutdown", e); } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java index 112654a7a07..7e9ff925251 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java @@ -190,12 +190,7 @@ public long getServerId() { } @Override - public synchronized void shutdown(boolean fullyShutDown) { - if (!canShutdown()) { - super.shutdown(fullyShutDown); - LOG.debug("ZooKeeper server is not running, so not proceeding to shutdown!"); - return; - } + protected void shutdownComponents() { shutdown = true; unregisterJMX(this); @@ -207,7 +202,7 @@ public synchronized void shutdown(boolean fullyShutDown) { self.adminServer.setZooKeeperServer(null); // shutdown the server itself - super.shutdown(fullyShutDown); + super.shutdownComponents(); } @Override diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/WatchLeakTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/WatchLeakTest.java index b51cfbe3ba2..8db90a2482e 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/WatchLeakTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/WatchLeakTest.java @@ -97,7 +97,7 @@ public Boolean answer(InvocationOnMock invocation) throws Throwable { } }); - ZKDatabase database = new ZKDatabase(null); + ZKDatabase database = new ZKDatabase(mock(FileTxnSnapLog.class)); database.setlastProcessedZxid(2L); QuorumPeer quorumPeer = mock(QuorumPeer.class); FileTxnSnapLog logfactory = mock(FileTxnSnapLog.class); From 3d6f61906451aa9d5f27051f605e54ddc7c71f80 Mon Sep 17 00:00:00 2001 From: Kezhu Wang Date: Fri, 20 Sep 2024 23:38:45 +0800 Subject: [PATCH 8/8] Fix spotbugs failure --- .../apache/zookeeper/server/quorum/LeaderZooKeeperServer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java index ef9b0737268..799b8f96148 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java @@ -155,7 +155,7 @@ protected void unregisterMetrics() { } @Override - protected void shutdownComponents() { + protected synchronized void shutdownComponents() { if (containerManager != null) { containerManager.stop(); }