ctubbsii commented on code in PR #5357:
URL: https://github.com/apache/accumulo/pull/5357#discussion_r1974224680


##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1231,6 +1232,23 @@ public void run() {
     final ServerContext context = getContext();
     final String zroot = context.getZooKeeperRoot();
 
+    // The following check will fail if an upgrade is in progress
+    // but the target version is not the current version of the
+    // software.
+    try {
+      if (UpgradeProgressTracker.upgradeInProgress(context)) {
+        try {
+          UpgradeProgressTracker.get(context);
+        } catch (IllegalStateException e) {
+          throw new IllegalStateException(
+              "Unable to start the Manager, upgrade in progress with a 
different version of software",
+              e);
+        }
+      }
+    } catch (KeeperException | InterruptedException e) {
+      throw new RuntimeException("Error checking for an existing upgrade", e);
+    }

Review Comment:
   Catching and throwing the same exception with a different message seems to 
suggest that it probably should have just thrown the first one with a better 
message in the first place, so there would be no need to catch and re-throw the 
same exception type.
   
   Also, could handle the Keeper/Interrupted exceptions inside the method, so 
that the ZK stuff doesn't leak this far out, and just throw an appropriate RTE 
(probably IllegalStateException) inside. the UpgradeProgressTracker methods.
   
   I think the code here might look better if it looked something like:
   
   ```java
       UpgradeProgressTracker.verifyNotInProgress();
       var progress = UpgradeProgressTracker.get();
   ```
   
   or
   
   ```java
       var progress = UpgradeProgressTracker.startUpgrade(); // throws 
exception if in progress for an incompatible version, and then later, just:
       var progress = UpgradeProgressTracker.continueUpgrade(); // this 
requires an upgrade to have started
   ```
   



##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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
+ *
+ *   https://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.accumulo.manager.upgrade;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.core.zookeeper.ZooSession;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.data.Stat;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+  /**
+   * Track upgrade progress for each component. The version stored is the most 
recent version for
+   * which an upgrade has been completed.
+   */
+  public static class UpgradeProgress {
+
+    private int zooKeeperVersion;
+    private int rootVersion;
+    private int metadataVersion;
+    private int upgradeTargetVersion;
+    private volatile transient int znodeVersion;
+
+    public UpgradeProgress() {}
+
+    public UpgradeProgress(int currentVersion, int targetVersion) {
+      zooKeeperVersion = currentVersion;
+      rootVersion = currentVersion;
+      metadataVersion = currentVersion;
+      upgradeTargetVersion = targetVersion;
+    }
+
+    public int getZooKeeperVersion() {
+      return zooKeeperVersion;
+    }
+
+    public synchronized void updateZooKeeperVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > zooKeeperVersion,
+          "New ZooKeeper version (%s) must be greater than current version 
(%s)", newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion > rootVersion,
+          "New ZooKeeper version (%s) expected to be greater than the root 
version (%s)",
+          newVersion, rootVersion);
+      checkArgument(metadataVersion == rootVersion,
+          "Root (%s) and Metadata (%s) versions expected to be equal when 
upgrading ZooKeeper",
+          rootVersion, metadataVersion);
+      zooKeeperVersion = newVersion;
+      put(context, this);
+    }
+
+    public int getRootVersion() {
+      return rootVersion;
+    }
+
+    public synchronized void updateRootVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > rootVersion,
+          "New Root version (%s) must be greater than current Root version 
(%s)", newVersion,
+          rootVersion);
+      checkArgument(newVersion <= zooKeeperVersion,
+          "New Root version (%s) expected to be <= ZooKeeper version (%s)", 
newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion > metadataVersion,
+          "New Root version (%s) must be greater than current Metadata version 
(%s)", newVersion,
+          metadataVersion);
+      rootVersion = newVersion;
+      put(context, this);
+    }
+
+    public int getMetadataVersion() {
+      return metadataVersion;
+    }
+
+    public synchronized void updateMetadataVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > metadataVersion,
+          "New Metadata version (%s) must be greater than current version 
(%s)", newVersion,
+          metadataVersion);
+      checkArgument(newVersion <= zooKeeperVersion,
+          "New Metadata version (%s) expected to be <= ZooKeeper version 
(%s)", newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion <= rootVersion,
+          "New Metadata version (%s) expected to be <= Root version (%s)", 
newVersion, rootVersion);
+      metadataVersion = newVersion;
+      put(context, this);
+    }
+  }
+
+  private static String getZPath(ServerContext context) {
+    return context.getZooKeeperRoot() + Constants.ZUPGRADE_PROGRESS;
+  }
+
+  public static boolean upgradeInProgress(ServerContext context)
+      throws KeeperException, InterruptedException {
+    requireNonNull(context, "ServerContext must be supplied");
+    final String zpath = getZPath(context);
+    final ZooSession zs = context.getZooSession();
+    return zs.exists(zpath, null) != null;
+  }
+
+  private static void put(ServerContext context, UpgradeProgress cv)
+      throws KeeperException, InterruptedException {
+    requireNonNull(context, "ServerContext must be supplied");
+    requireNonNull(cv, "ComponentVersions object  must be supplied");
+    final String zpath = getZPath(context);
+    final ZooSession zs = context.getZooSession();
+    Stat stat = zs.exists(zpath, null);
+    if (stat == null) {
+      zs.create(zpath, GSON.get().toJson(cv).getBytes(UTF_8), ZooUtil.PUBLIC,
+          CreateMode.PERSISTENT);
+      cv.znodeVersion = zs.exists(zpath, null).getVersion();

Review Comment:
   If create succeeds without error, then the version is always going to be `0`.



##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1231,6 +1232,23 @@ public void run() {
     final ServerContext context = getContext();
     final String zroot = context.getZooKeeperRoot();
 
+    // The following check will fail if an upgrade is in progress
+    // but the target version is not the current version of the
+    // software.
+    try {
+      if (UpgradeProgressTracker.upgradeInProgress(context)) {
+        try {
+          UpgradeProgressTracker.get(context);

Review Comment:
   Are you not using it here? The return variable is ignored. Should this just 
be deferred to when the UpgradeCoordinator is constructed?



##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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
+ *
+ *   https://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.accumulo.manager.upgrade;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.core.zookeeper.ZooSession;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.data.Stat;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+  /**
+   * Track upgrade progress for each component. The version stored is the most 
recent version for
+   * which an upgrade has been completed.
+   */
+  public static class UpgradeProgress {
+
+    private int zooKeeperVersion;
+    private int rootVersion;
+    private int metadataVersion;
+    private int upgradeTargetVersion;
+    private volatile transient int znodeVersion;
+
+    public UpgradeProgress() {}
+
+    public UpgradeProgress(int currentVersion, int targetVersion) {
+      zooKeeperVersion = currentVersion;
+      rootVersion = currentVersion;
+      metadataVersion = currentVersion;
+      upgradeTargetVersion = targetVersion;
+    }
+
+    public int getZooKeeperVersion() {
+      return zooKeeperVersion;
+    }
+
+    public synchronized void updateZooKeeperVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > zooKeeperVersion,
+          "New ZooKeeper version (%s) must be greater than current version 
(%s)", newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion > rootVersion,
+          "New ZooKeeper version (%s) expected to be greater than the root 
version (%s)",
+          newVersion, rootVersion);
+      checkArgument(metadataVersion == rootVersion,
+          "Root (%s) and Metadata (%s) versions expected to be equal when 
upgrading ZooKeeper",
+          rootVersion, metadataVersion);
+      zooKeeperVersion = newVersion;
+      put(context, this);
+    }
+
+    public int getRootVersion() {
+      return rootVersion;
+    }
+
+    public synchronized void updateRootVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > rootVersion,
+          "New Root version (%s) must be greater than current Root version 
(%s)", newVersion,
+          rootVersion);
+      checkArgument(newVersion <= zooKeeperVersion,
+          "New Root version (%s) expected to be <= ZooKeeper version (%s)", 
newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion > metadataVersion,
+          "New Root version (%s) must be greater than current Metadata version 
(%s)", newVersion,
+          metadataVersion);
+      rootVersion = newVersion;
+      put(context, this);
+    }
+
+    public int getMetadataVersion() {
+      return metadataVersion;
+    }
+
+    public synchronized void updateMetadataVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > metadataVersion,
+          "New Metadata version (%s) must be greater than current version 
(%s)", newVersion,
+          metadataVersion);
+      checkArgument(newVersion <= zooKeeperVersion,
+          "New Metadata version (%s) expected to be <= ZooKeeper version 
(%s)", newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion <= rootVersion,
+          "New Metadata version (%s) expected to be <= Root version (%s)", 
newVersion, rootVersion);
+      metadataVersion = newVersion;
+      put(context, this);
+    }
+  }
+
+  private static String getZPath(ServerContext context) {
+    return context.getZooKeeperRoot() + Constants.ZUPGRADE_PROGRESS;
+  }
+
+  public static boolean upgradeInProgress(ServerContext context)
+      throws KeeperException, InterruptedException {
+    requireNonNull(context, "ServerContext must be supplied");
+    final String zpath = getZPath(context);
+    final ZooSession zs = context.getZooSession();
+    return zs.exists(zpath, null) != null;
+  }
+
+  private static void put(ServerContext context, UpgradeProgress cv)

Review Comment:
   If this is made non-static, you can stop passing the `this` parameter.



##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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
+ *
+ *   https://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.accumulo.manager.upgrade;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.core.zookeeper.ZooSession;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.data.Stat;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+  /**
+   * Track upgrade progress for each component. The version stored is the most 
recent version for
+   * which an upgrade has been completed.
+   */
+  public static class UpgradeProgress {
+
+    private int zooKeeperVersion;
+    private int rootVersion;
+    private int metadataVersion;
+    private int upgradeTargetVersion;
+    private volatile transient int znodeVersion;
+
+    public UpgradeProgress() {}
+
+    public UpgradeProgress(int currentVersion, int targetVersion) {
+      zooKeeperVersion = currentVersion;
+      rootVersion = currentVersion;
+      metadataVersion = currentVersion;
+      upgradeTargetVersion = targetVersion;
+    }
+
+    public int getZooKeeperVersion() {
+      return zooKeeperVersion;
+    }
+
+    public synchronized void updateZooKeeperVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > zooKeeperVersion,
+          "New ZooKeeper version (%s) must be greater than current version 
(%s)", newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion > rootVersion,
+          "New ZooKeeper version (%s) expected to be greater than the root 
version (%s)",
+          newVersion, rootVersion);
+      checkArgument(metadataVersion == rootVersion,
+          "Root (%s) and Metadata (%s) versions expected to be equal when 
upgrading ZooKeeper",
+          rootVersion, metadataVersion);
+      zooKeeperVersion = newVersion;
+      put(context, this);
+    }
+
+    public int getRootVersion() {
+      return rootVersion;
+    }
+
+    public synchronized void updateRootVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > rootVersion,
+          "New Root version (%s) must be greater than current Root version 
(%s)", newVersion,
+          rootVersion);
+      checkArgument(newVersion <= zooKeeperVersion,
+          "New Root version (%s) expected to be <= ZooKeeper version (%s)", 
newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion > metadataVersion,
+          "New Root version (%s) must be greater than current Metadata version 
(%s)", newVersion,
+          metadataVersion);
+      rootVersion = newVersion;
+      put(context, this);
+    }
+
+    public int getMetadataVersion() {
+      return metadataVersion;
+    }
+
+    public synchronized void updateMetadataVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > metadataVersion,
+          "New Metadata version (%s) must be greater than current version 
(%s)", newVersion,
+          metadataVersion);
+      checkArgument(newVersion <= zooKeeperVersion,
+          "New Metadata version (%s) expected to be <= ZooKeeper version 
(%s)", newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion <= rootVersion,
+          "New Metadata version (%s) expected to be <= Root version (%s)", 
newVersion, rootVersion);
+      metadataVersion = newVersion;
+      put(context, this);
+    }
+  }
+
+  private static String getZPath(ServerContext context) {
+    return context.getZooKeeperRoot() + Constants.ZUPGRADE_PROGRESS;
+  }
+
+  public static boolean upgradeInProgress(ServerContext context)
+      throws KeeperException, InterruptedException {
+    requireNonNull(context, "ServerContext must be supplied");
+    final String zpath = getZPath(context);
+    final ZooSession zs = context.getZooSession();
+    return zs.exists(zpath, null) != null;
+  }
+
+  private static void put(ServerContext context, UpgradeProgress cv)
+      throws KeeperException, InterruptedException {
+    requireNonNull(context, "ServerContext must be supplied");
+    requireNonNull(cv, "ComponentVersions object  must be supplied");
+    final String zpath = getZPath(context);
+    final ZooSession zs = context.getZooSession();
+    Stat stat = zs.exists(zpath, null);
+    if (stat == null) {
+      zs.create(zpath, GSON.get().toJson(cv).getBytes(UTF_8), ZooUtil.PUBLIC,
+          CreateMode.PERSISTENT);
+      cv.znodeVersion = zs.exists(zpath, null).getVersion();
+    } else {
+      try {
+        zs.setData(zpath, GSON.get().toJson(cv).getBytes(UTF_8), 
cv.znodeVersion);

Review Comment:
   This returns a new Stat object with the updated znodeVersion. I think you 
could update it here. The only time you need to set it when it already exists 
is on the initial state if the node already exists from a previous upgrade. So, 
you could set it when you first check if the node exists and doesn't have an 
incompatible upgrade target version, and then set it again, any time you call 
setData, using setData's returned Stat object, rather on each call to `get()`. 
I'm not sure if that would be cleaner, or more intuitive, though.



##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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
+ *
+ *   https://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.accumulo.manager.upgrade;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.core.zookeeper.ZooSession;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.data.Stat;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+  /**
+   * Track upgrade progress for each component. The version stored is the most 
recent version for
+   * which an upgrade has been completed.
+   */
+  public static class UpgradeProgress {
+
+    private int zooKeeperVersion;
+    private int rootVersion;
+    private int metadataVersion;
+    private int upgradeTargetVersion;
+    private volatile transient int znodeVersion;
+
+    public UpgradeProgress() {}
+
+    public UpgradeProgress(int currentVersion, int targetVersion) {
+      zooKeeperVersion = currentVersion;
+      rootVersion = currentVersion;
+      metadataVersion = currentVersion;
+      upgradeTargetVersion = targetVersion;
+    }
+
+    public int getZooKeeperVersion() {
+      return zooKeeperVersion;
+    }
+
+    public synchronized void updateZooKeeperVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > zooKeeperVersion,
+          "New ZooKeeper version (%s) must be greater than current version 
(%s)", newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion > rootVersion,
+          "New ZooKeeper version (%s) expected to be greater than the root 
version (%s)",
+          newVersion, rootVersion);
+      checkArgument(metadataVersion == rootVersion,
+          "Root (%s) and Metadata (%s) versions expected to be equal when 
upgrading ZooKeeper",
+          rootVersion, metadataVersion);
+      zooKeeperVersion = newVersion;
+      put(context, this);
+    }
+
+    public int getRootVersion() {
+      return rootVersion;
+    }
+
+    public synchronized void updateRootVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > rootVersion,
+          "New Root version (%s) must be greater than current Root version 
(%s)", newVersion,
+          rootVersion);
+      checkArgument(newVersion <= zooKeeperVersion,
+          "New Root version (%s) expected to be <= ZooKeeper version (%s)", 
newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion > metadataVersion,
+          "New Root version (%s) must be greater than current Metadata version 
(%s)", newVersion,
+          metadataVersion);
+      rootVersion = newVersion;
+      put(context, this);
+    }
+
+    public int getMetadataVersion() {
+      return metadataVersion;
+    }
+
+    public synchronized void updateMetadataVersion(ServerContext context, int 
newVersion)
+        throws KeeperException, InterruptedException {
+      requireNonNull(context, "ServerContext must be supplied");
+      checkArgument(newVersion <= AccumuloDataVersion.get(),
+          "New version (%s) cannot be larger than current data version (%s)", 
newVersion,
+          AccumuloDataVersion.get());
+      checkArgument(newVersion > metadataVersion,
+          "New Metadata version (%s) must be greater than current version 
(%s)", newVersion,
+          metadataVersion);
+      checkArgument(newVersion <= zooKeeperVersion,
+          "New Metadata version (%s) expected to be <= ZooKeeper version 
(%s)", newVersion,
+          zooKeeperVersion);
+      checkArgument(newVersion <= rootVersion,
+          "New Metadata version (%s) expected to be <= Root version (%s)", 
newVersion, rootVersion);
+      metadataVersion = newVersion;
+      put(context, this);
+    }
+  }
+
+  private static String getZPath(ServerContext context) {
+    return context.getZooKeeperRoot() + Constants.ZUPGRADE_PROGRESS;
+  }
+
+  public static boolean upgradeInProgress(ServerContext context)
+      throws KeeperException, InterruptedException {
+    requireNonNull(context, "ServerContext must be supplied");
+    final String zpath = getZPath(context);
+    final ZooSession zs = context.getZooSession();
+    return zs.exists(zpath, null) != null;
+  }
+
+  private static void put(ServerContext context, UpgradeProgress cv)
+      throws KeeperException, InterruptedException {
+    requireNonNull(context, "ServerContext must be supplied");
+    requireNonNull(cv, "ComponentVersions object  must be supplied");
+    final String zpath = getZPath(context);
+    final ZooSession zs = context.getZooSession();
+    Stat stat = zs.exists(zpath, null);
+    if (stat == null) {
+      zs.create(zpath, GSON.get().toJson(cv).getBytes(UTF_8), ZooUtil.PUBLIC,
+          CreateMode.PERSISTENT);
+      cv.znodeVersion = zs.exists(zpath, null).getVersion();
+    } else {
+      try {
+        zs.setData(zpath, GSON.get().toJson(cv).getBytes(UTF_8), 
cv.znodeVersion);
+      } catch (KeeperException e) {
+        if (e.code() == KeeperException.Code.BADVERSION) {

Review Comment:
   Each separate code has it's own KeeperException subclass. You could catch 
the one for KeeperException.BadVersionException directly in an earlier catch 
clause, so you don't have to do this check of the code.



-- 
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]

Reply via email to