ctubbsii commented on code in PR #5357:
URL: https://github.com/apache/accumulo/pull/5357#discussion_r1970683778
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -173,13 +174,22 @@ public synchronized void upgradeZookeeper(ServerContext
context,
if (currentVersion < AccumuloDataVersion.get()) {
abortIfFateTransactions(context);
+ final ComponentVersions tracker = UpgradeProgressTracker.get(context);
Review Comment:
Slight improvement: if it's tracking UpgradeProgress, then make that the
return type:
```suggestion
final UpgradeProgress progress = UpgradeProgressTracker.get(context);
```
or maybe:
```suggestion
final UpgradeProgress progress = UpgradeProgress.track(context);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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 java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+ public static class ComponentVersions {
+ private int zooKeeperVersion;
+ private int rootVersion;
+ private int metadataVersion;
+
+ public int getZooKeeperVersion() {
+ return zooKeeperVersion;
+ }
+
+ public synchronized void updateZooKeeperVersion(ServerContext context, int
newVersion)
+ throws KeeperException, InterruptedException {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > zooKeeperVersion,
+ "New ZooKeeper version (%s) must be greater than current version
(%s)", newVersion,
+ zooKeeperVersion);
+ Preconditions.checkArgument(newVersion > rootVersion,
+ "New ZooKeeper version (%s) expected to be greater than the root
version (%s)",
+ newVersion, rootVersion);
+ Preconditions.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 {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > rootVersion,
+ "New Root version (%s) must be greater than current Root version
(%s)", newVersion,
+ rootVersion);
+ Preconditions.checkArgument(newVersion <= zooKeeperVersion,
+ "New Root version (%s) expected to be <= ZooKeeper version (%s)",
newVersion,
+ zooKeeperVersion);
+ Preconditions.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 {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > metadataVersion,
+ "New Metadata version (%s) must be greater than current version
(%s)", newVersion,
+ metadataVersion);
+ Preconditions.checkArgument(newVersion <= zooKeeperVersion,
+ "New Metadata version (%s) expected to be <= ZooKeeper version
(%s)", newVersion,
+ zooKeeperVersion);
+ Preconditions.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_STATUS;
+ }
+
+ private static synchronized void put(ServerContext context,
ComponentVersions cv)
+ throws KeeperException, InterruptedException {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Objects.requireNonNull(cv, "ComponentVersions object must be supplied");
+ final String zpath = getZPath(context);
+ final ZooReaderWriter zrw = context.getZooSession().asReaderWriter();
+ zrw.sync(zpath);
+ if (!zrw.exists(zpath)) {
+ zrw.mkdirs(zpath);
+ }
+ zrw.putPersistentData(zpath, GSON.get().toJson(cv).getBytes(UTF_8),
NodeExistsPolicy.OVERWRITE);
+ }
+
+ public static synchronized ComponentVersions get(ServerContext context)
+ throws KeeperException, InterruptedException {
+ final String zpath = getZPath(context);
+ final int currentVersion = AccumuloDataVersion.getCurrentVersion(context);
+ final ZooReaderWriter zrw = context.getZooSession().asReaderWriter();
+ zrw.sync(zpath);
+ if (!zrw.exists(zpath)) {
+ ComponentVersions cv = new ComponentVersions();
+ cv.zooKeeperVersion = currentVersion;
+ cv.rootVersion = currentVersion;
+ cv.metadataVersion = currentVersion;
Review Comment:
This could just be an overloaded constructor that takes the version... or
better yet, a static method on the object that initializes it to the current
version:
```suggestion
var cv = ComponentVersions.fromCurrentVersion(currentVersion);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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 java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+ public static class ComponentVersions {
+ private int zooKeeperVersion;
+ private int rootVersion;
+ private int metadataVersion;
+
+ public int getZooKeeperVersion() {
+ return zooKeeperVersion;
+ }
+
+ public synchronized void updateZooKeeperVersion(ServerContext context, int
newVersion)
+ throws KeeperException, InterruptedException {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
Review Comment:
These methods are used a lot. I'd suggest considering using static imports
on them. They are well-known methods, so I don't think there'd be any confusion
about where they are coming from, and it might make the code easier to read.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -173,13 +174,22 @@ public synchronized void upgradeZookeeper(ServerContext
context,
if (currentVersion < AccumuloDataVersion.get()) {
abortIfFateTransactions(context);
+ final ComponentVersions tracker = UpgradeProgressTracker.get(context);
+
for (int v = currentVersion; v < AccumuloDataVersion.get(); v++) {
+ if (tracker.getZooKeeperVersion() >= currentVersion) {
Review Comment:
```suggestion
if (progress.getZooKeeperVersion() >= currentVersion) {
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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 java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+ public static class ComponentVersions {
Review Comment:
```suggestion
/**
* 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 {
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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 java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+ public static class ComponentVersions {
+ private int zooKeeperVersion;
+ private int rootVersion;
+ private int metadataVersion;
+
+ public int getZooKeeperVersion() {
+ return zooKeeperVersion;
+ }
+
+ public synchronized void updateZooKeeperVersion(ServerContext context, int
newVersion)
+ throws KeeperException, InterruptedException {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > zooKeeperVersion,
+ "New ZooKeeper version (%s) must be greater than current version
(%s)", newVersion,
+ zooKeeperVersion);
+ Preconditions.checkArgument(newVersion > rootVersion,
+ "New ZooKeeper version (%s) expected to be greater than the root
version (%s)",
+ newVersion, rootVersion);
+ Preconditions.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 {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > rootVersion,
+ "New Root version (%s) must be greater than current Root version
(%s)", newVersion,
+ rootVersion);
+ Preconditions.checkArgument(newVersion <= zooKeeperVersion,
+ "New Root version (%s) expected to be <= ZooKeeper version (%s)",
newVersion,
+ zooKeeperVersion);
+ Preconditions.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 {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > metadataVersion,
+ "New Metadata version (%s) must be greater than current version
(%s)", newVersion,
+ metadataVersion);
+ Preconditions.checkArgument(newVersion <= zooKeeperVersion,
+ "New Metadata version (%s) expected to be <= ZooKeeper version
(%s)", newVersion,
+ zooKeeperVersion);
+ Preconditions.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_STATUS;
+ }
+
+ private static synchronized void put(ServerContext context,
ComponentVersions cv)
+ throws KeeperException, InterruptedException {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Objects.requireNonNull(cv, "ComponentVersions object must be supplied");
+ final String zpath = getZPath(context);
+ final ZooReaderWriter zrw = context.getZooSession().asReaderWriter();
+ zrw.sync(zpath);
+ if (!zrw.exists(zpath)) {
+ zrw.mkdirs(zpath);
+ }
+ zrw.putPersistentData(zpath, GSON.get().toJson(cv).getBytes(UTF_8),
NodeExistsPolicy.OVERWRITE);
+ }
+
+ public static synchronized ComponentVersions get(ServerContext context)
+ throws KeeperException, InterruptedException {
+ final String zpath = getZPath(context);
+ final int currentVersion = AccumuloDataVersion.getCurrentVersion(context);
+ final ZooReaderWriter zrw = context.getZooSession().asReaderWriter();
+ zrw.sync(zpath);
+ if (!zrw.exists(zpath)) {
+ ComponentVersions cv = new ComponentVersions();
+ cv.zooKeeperVersion = currentVersion;
+ cv.rootVersion = currentVersion;
+ cv.metadataVersion = currentVersion;
+ put(context, cv);
+ return cv;
+ } else {
+ byte[] jsonData = zrw.getData(zpath);
+ return GSON.get().fromJson(new String(jsonData, UTF_8),
ComponentVersions.class);
+ }
+ }
+
+ public static synchronized void upgradeComplete(ServerContext context)
+ throws KeeperException, InterruptedException {
+ // This should be updated prior to deleting the tracking data in zookeeper.
+ Preconditions
+ .checkState(AccumuloDataVersion.getCurrentVersion(context) ==
AccumuloDataVersion.get());
+ final String zpath = getZPath(context);
+ final ZooReaderWriter zrw = context.getZooSession().asReaderWriter();
+ zrw.sync(zpath);
+ zrw.recursiveDelete(zpath, NodeMissingPolicy.SKIP);
Review Comment:
You don't need to sync because write operations already require quorum.
```suggestion
zrw.recursiveDelete(zpath, NodeMissingPolicy.SKIP);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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 java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+ public static class ComponentVersions {
+ private int zooKeeperVersion;
+ private int rootVersion;
+ private int metadataVersion;
+
+ public int getZooKeeperVersion() {
+ return zooKeeperVersion;
+ }
+
+ public synchronized void updateZooKeeperVersion(ServerContext context, int
newVersion)
+ throws KeeperException, InterruptedException {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > zooKeeperVersion,
+ "New ZooKeeper version (%s) must be greater than current version
(%s)", newVersion,
+ zooKeeperVersion);
+ Preconditions.checkArgument(newVersion > rootVersion,
+ "New ZooKeeper version (%s) expected to be greater than the root
version (%s)",
+ newVersion, rootVersion);
+ Preconditions.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 {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > rootVersion,
+ "New Root version (%s) must be greater than current Root version
(%s)", newVersion,
+ rootVersion);
+ Preconditions.checkArgument(newVersion <= zooKeeperVersion,
+ "New Root version (%s) expected to be <= ZooKeeper version (%s)",
newVersion,
+ zooKeeperVersion);
+ Preconditions.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 {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > metadataVersion,
+ "New Metadata version (%s) must be greater than current version
(%s)", newVersion,
+ metadataVersion);
+ Preconditions.checkArgument(newVersion <= zooKeeperVersion,
+ "New Metadata version (%s) expected to be <= ZooKeeper version
(%s)", newVersion,
+ zooKeeperVersion);
+ Preconditions.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_STATUS;
+ }
+
+ private static synchronized void put(ServerContext context,
ComponentVersions cv)
+ throws KeeperException, InterruptedException {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Objects.requireNonNull(cv, "ComponentVersions object must be supplied");
+ final String zpath = getZPath(context);
+ final ZooReaderWriter zrw = context.getZooSession().asReaderWriter();
+ zrw.sync(zpath);
+ if (!zrw.exists(zpath)) {
+ zrw.mkdirs(zpath);
+ }
+ zrw.putPersistentData(zpath, GSON.get().toJson(cv).getBytes(UTF_8),
NodeExistsPolicy.OVERWRITE);
+ }
+
+ public static synchronized ComponentVersions get(ServerContext context)
+ throws KeeperException, InterruptedException {
+ final String zpath = getZPath(context);
+ final int currentVersion = AccumuloDataVersion.getCurrentVersion(context);
+ final ZooReaderWriter zrw = context.getZooSession().asReaderWriter();
+ zrw.sync(zpath);
+ if (!zrw.exists(zpath)) {
+ ComponentVersions cv = new ComponentVersions();
+ cv.zooKeeperVersion = currentVersion;
+ cv.rootVersion = currentVersion;
+ cv.metadataVersion = currentVersion;
+ put(context, cv);
+ return cv;
+ } else {
+ byte[] jsonData = zrw.getData(zpath);
+ return GSON.get().fromJson(new String(jsonData, UTF_8),
ComponentVersions.class);
+ }
+ }
+
+ public static synchronized void upgradeComplete(ServerContext context)
+ throws KeeperException, InterruptedException {
+ // This should be updated prior to deleting the tracking data in zookeeper.
+ Preconditions
+ .checkState(AccumuloDataVersion.getCurrentVersion(context) ==
AccumuloDataVersion.get());
Review Comment:
This could use a custom error message for when it fails.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,127 @@
+/*
+ * 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 java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.base.Preconditions;
+
+public class UpgradeProgressTracker {
+
+ public static class ComponentVersions {
+ private int zooKeeperVersion;
+ private int rootVersion;
+ private int metadataVersion;
Review Comment:
We don't necessarily need to track a target version here. If we didn't, we
could still check that any completed upgrade versions are not newer than what
the current code supports.
However, if we don't track a target version, then we can't be sure that
there wasn't a partial upgrade attempt after the most recently recorded
successful upgrade version, that is now no longer possible to recover from. So,
the target version is kind of important... or at least some insight into
whether the most recently recorded version is actually a terminal state at the
end of any upgrade efforts.
For example, let's say we fail while upgrading ZooKeeper, going from 2.1 to
4.0, and it failed after we upgrade ZooKeeper to 3.1. If we don't track the
target version, and we don't know for sure that 3.1 was actually the terminal
condition we were trying to reach, then we can't really be sure that the
failure didn't occur after making some changes from 3.1 to 4.0. If we then try
to start up 3.1, it may believe it can complete the upgrade to 3.1, when the
upgrade progress has already moved a little past 3.1 in an attempt to get it to
4.0. So, we would need to track either a target version, or at least know that
no additional work was done after the successful update to 3.1.
We could increment the target version each time before starting an upgrade
stage, though. That would ensure that a partial upgrade to 4.0 that is
interrupted after getting as far as 3.1 could be completed to 3.1 by the 3.1
code, if it hadn't yet started the 3.1 to 4.0 transition.
For example, considering just the ZK upgrade loop going from version A to C:
1. Bump ZK target version to B
2. Upgrade ZK from A to B
3. Set ZK completed version to B
4. Bump ZK target version to C
5. Upgrade ZK from B to C
6. Set ZK completed version to C
This means that if the upgrade was interrupted between step 3 and 4 using
Accumulo version C, Accumulo version B could still complete an upgrade to B.
I think what @keith-turner was proposing is more of a "final target
version", in which case only version C could complete the upgrade that version
C started, even if it hadn't gotten to B yet. We can call this algorithm I've
outlined here the "next target version" model, instead of the "final target
version" model, so you just increment the target to the next step. I don't know
if it's worth it, but it could be helpful if version C had a problem that
version B didn't, and version B was able to at least complete an upgrade to B,
to get Accumulo back into a useful state.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -173,13 +174,22 @@ public synchronized void upgradeZookeeper(ServerContext
context,
if (currentVersion < AccumuloDataVersion.get()) {
abortIfFateTransactions(context);
+ final ComponentVersions tracker = UpgradeProgressTracker.get(context);
+
for (int v = currentVersion; v < AccumuloDataVersion.get(); v++) {
+ if (tracker.getZooKeeperVersion() >= currentVersion) {
+ log.info(
+ "ZooKeeper has already been upgraded to version {}, moving on
to next upgrader",
+ currentVersion);
+ continue;
+ }
log.info("Upgrading Zookeeper - current version {} as step towards
target version {}", v,
AccumuloDataVersion.get());
var upgrader = upgraders.get(v);
Objects.requireNonNull(upgrader,
"upgrade ZooKeeper: failed to find upgrader for version " +
currentVersion);
upgrader.upgradeZookeeper(context);
+ tracker.updateZooKeeperVersion(context, v);
Review Comment:
```suggestion
progress.updateZooKeeperVersion(context, v);
```
##########
core/src/main/java/org/apache/accumulo/core/Constants.java:
##########
@@ -84,6 +84,8 @@ public class Constants {
public static final String ZHDFS_RESERVATIONS = "/hdfs_reservations";
public static final String ZRECOVERY = "/recovery";
+ public static final String ZUPGRADE_STATUS = "/upgrade_status";
Review Comment:
```suggestion
public static final String ZUPGRADE_PROGRESS = "/upgrade_progress";
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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 java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+ public static class ComponentVersions {
+ private int zooKeeperVersion;
+ private int rootVersion;
+ private int metadataVersion;
+
+ public int getZooKeeperVersion() {
+ return zooKeeperVersion;
+ }
Review Comment:
There's a lot of synchronization on writing these versions, but none on the
reads, and the fields are not volatile or atomic, so they could be out of date
in some threads.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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 java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+ public static class ComponentVersions {
+ private int zooKeeperVersion;
+ private int rootVersion;
+ private int metadataVersion;
+
+ public int getZooKeeperVersion() {
+ return zooKeeperVersion;
+ }
+
+ public synchronized void updateZooKeeperVersion(ServerContext context, int
newVersion)
+ throws KeeperException, InterruptedException {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > zooKeeperVersion,
+ "New ZooKeeper version (%s) must be greater than current version
(%s)", newVersion,
+ zooKeeperVersion);
+ Preconditions.checkArgument(newVersion > rootVersion,
+ "New ZooKeeper version (%s) expected to be greater than the root
version (%s)",
+ newVersion, rootVersion);
+ Preconditions.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 {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > rootVersion,
+ "New Root version (%s) must be greater than current Root version
(%s)", newVersion,
+ rootVersion);
+ Preconditions.checkArgument(newVersion <= zooKeeperVersion,
+ "New Root version (%s) expected to be <= ZooKeeper version (%s)",
newVersion,
+ zooKeeperVersion);
+ Preconditions.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 {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > metadataVersion,
+ "New Metadata version (%s) must be greater than current version
(%s)", newVersion,
+ metadataVersion);
+ Preconditions.checkArgument(newVersion <= zooKeeperVersion,
+ "New Metadata version (%s) expected to be <= ZooKeeper version
(%s)", newVersion,
+ zooKeeperVersion);
+ Preconditions.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_STATUS;
+ }
+
+ private static synchronized void put(ServerContext context,
ComponentVersions cv)
+ throws KeeperException, InterruptedException {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Objects.requireNonNull(cv, "ComponentVersions object must be supplied");
+ final String zpath = getZPath(context);
+ final ZooReaderWriter zrw = context.getZooSession().asReaderWriter();
+ zrw.sync(zpath);
+ if (!zrw.exists(zpath)) {
+ zrw.mkdirs(zpath);
+ }
+ zrw.putPersistentData(zpath, GSON.get().toJson(cv).getBytes(UTF_8),
NodeExistsPolicy.OVERWRITE);
+ }
+
+ public static synchronized ComponentVersions get(ServerContext context)
+ throws KeeperException, InterruptedException {
+ final String zpath = getZPath(context);
+ final int currentVersion = AccumuloDataVersion.getCurrentVersion(context);
+ final ZooReaderWriter zrw = context.getZooSession().asReaderWriter();
+ zrw.sync(zpath);
+ if (!zrw.exists(zpath)) {
Review Comment:
I think this code will change a bit if my other suggestions are followed.
But, you don't need to sync here. Instead, you can attempt to create the node
(since most of the time, that's the usual case). Creating the node requires
quorum, so you don't have to worry about calling sync, and it protects you
better, since the client could become unsynced immediately after asking it to
sync. If creating the node fails because it already exists, then you can parse
it and determine whether you should resume or abort, based on what's there.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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 java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.util.LazySingletons.GSON;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeExistsPolicy;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil.NodeMissingPolicy;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * Methods in this class are public **only** for UpgradeProgressTrackerIT
+ */
+public class UpgradeProgressTracker {
+
+ public static class ComponentVersions {
+ private int zooKeeperVersion;
+ private int rootVersion;
+ private int metadataVersion;
+
+ public int getZooKeeperVersion() {
+ return zooKeeperVersion;
+ }
+
+ public synchronized void updateZooKeeperVersion(ServerContext context, int
newVersion)
+ throws KeeperException, InterruptedException {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > zooKeeperVersion,
+ "New ZooKeeper version (%s) must be greater than current version
(%s)", newVersion,
+ zooKeeperVersion);
+ Preconditions.checkArgument(newVersion > rootVersion,
+ "New ZooKeeper version (%s) expected to be greater than the root
version (%s)",
+ newVersion, rootVersion);
+ Preconditions.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 {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > rootVersion,
+ "New Root version (%s) must be greater than current Root version
(%s)", newVersion,
+ rootVersion);
+ Preconditions.checkArgument(newVersion <= zooKeeperVersion,
+ "New Root version (%s) expected to be <= ZooKeeper version (%s)",
newVersion,
+ zooKeeperVersion);
+ Preconditions.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 {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Preconditions.checkArgument(newVersion <= AccumuloDataVersion.get(),
+ "New version (%s) cannot be larger than current data version (%s)",
newVersion,
+ AccumuloDataVersion.get());
+ Preconditions.checkArgument(newVersion > metadataVersion,
+ "New Metadata version (%s) must be greater than current version
(%s)", newVersion,
+ metadataVersion);
+ Preconditions.checkArgument(newVersion <= zooKeeperVersion,
+ "New Metadata version (%s) expected to be <= ZooKeeper version
(%s)", newVersion,
+ zooKeeperVersion);
+ Preconditions.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_STATUS;
+ }
+
+ private static synchronized void put(ServerContext context,
ComponentVersions cv)
+ throws KeeperException, InterruptedException {
+ Objects.requireNonNull(context, "ServerContext must be supplied");
+ Objects.requireNonNull(cv, "ComponentVersions object must be supplied");
+ final String zpath = getZPath(context);
+ final ZooReaderWriter zrw = context.getZooSession().asReaderWriter();
+ zrw.sync(zpath);
+ if (!zrw.exists(zpath)) {
+ zrw.mkdirs(zpath);
+ }
+ zrw.putPersistentData(zpath, GSON.get().toJson(cv).getBytes(UTF_8),
NodeExistsPolicy.OVERWRITE);
Review Comment:
You don't need this mkdirs... the parent nodes should already be created,
and the node itself will be created when you put the data in the node. You also
don't need the sync, as you only need the write operation here and that already
requires quorum.
Instead of this put method using ZRW, you should use the ZooSession methods
directly that allow you to specify the expected current version when updating
the node contents, and setting the Stat object when reading, so you can capture
the initial version of the node if it already exists. Then you can use the
expected zookeeper node version when you call setData to ensure that no
collisions occur. An unexpected version (BadVersion KeeperException) indicates
that the node changed unexpected by another process or thread, and you don't
need all the synchronization to protect against that.
--
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]
