jonmv commented on code in PR #2154:
URL: https://github.com/apache/zookeeper/pull/2154#discussion_r1768507441


##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java:
##########
@@ -924,27 +924,49 @@ public boolean isRunning() {
         return state == State.RUNNING;
     }
 
-    public void shutdown() {
+    public final void shutdown() {
         shutdown(false);
     }
 
     /**
      * 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();

Review Comment:
   The metrics used by the `FileTxnSnapLog` are not "owned" by the 
`ZooKeeperServer`, or its children, and not unregistered here, so that 
shouldn't be a problem. 
   On the other hand, as the parent `ZooKeeperServer` is a dependency of its 
child classes, I would generally shut down all _their_ components _before_ the 
parent, and I consider the `zkDb` to be owned by the parent in this case, which 
suggests the current order of shutdown is correct. That said, "shutdown is 
hard" 😂 



-- 
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: notifications-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to