You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2023/01/13 19:59:37 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3160: remove upgrade code for versions before 2.1
EdColeman opened a new pull request, #3160:
URL: https://github.com/apache/accumulo/pull/3160
3.x will only support upgrades from 2.1, these changes remove code that supports upgrade before 2.1
- add upgrade check to allow for validation on upgrades - include ZK ACL check
- removes Accumulo data versions < 10
- removes upgraders 8-9 and 9-10
- removes associated tests
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman merged pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman merged PR #3160:
URL: https://github.com/apache/accumulo/pull/3160
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1099329396
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +145,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = upgraders.entrySet().iterator().next().getKey();
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
+ + " data version (" + oldestVersion + ") is not supported. Upgrade to at least "
+ + oldRelease + " before upgrading to " + Constants.VERSION);
+ }
+
Review Comment:
Here's my proposed changes:
```diff
diff --git a/server/base/src/main/java/org/apache/accumulo/server/AccumuloDataVersion.java b/server/base/src/main/java/org/apache/accumulo/server/AccumuloDataVersion.java
index 9f523e7082..5b44913f11 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/AccumuloDataVersion.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/AccumuloDataVersion.java
@@ -86,7 +86,11 @@ public class AccumuloDataVersion {
return cv;
}
- public static String dataVersionToReleaseName(final int version) {
+ public static String oldestUpgradeableVersionName() {
+ return dataVersionToReleaseName(CAN_RUN.stream().mapToInt(x -> x).min().orElseThrow());
+ }
+
+ private static String dataVersionToReleaseName(final int version) {
switch (version) {
case ROOT_TABLET_META_CHANGES:
return "2.1.0";
diff --git a/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java b/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
index 03ab556923..e2286115c2 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
@@ -300,7 +300,9 @@ public class ServerContext extends ClientContext {
public static void ensureDataVersionCompatible(int dataVersion) {
if (!AccumuloDataVersion.CAN_RUN.contains(dataVersion)) {
throw new IllegalStateException("This version of accumulo (" + Constants.VERSION
- + ") is not compatible with files stored using data version " + dataVersion);
+ + ") is not compatible with files stored using data version " + dataVersion
+ + ". Please upgrade from " + AccumuloDataVersion.oldestUpgradeableVersionName()
+ + " or later.");
}
}
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1087062854
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeChecker.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * 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 java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.manager.EventCoordinator;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZKUtil;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class UpgradeChecker {
Review Comment:
This is not so much new code as it is moving existing code to a different place.
The code to upgrade to 2.1 included the check for ZooKeeper ACLs and with the deletion of that code, this check would not be performed. Being that it is unclear how the invalid ACLs are occurring, it seemed like something that could be checked on each upgrade and not pegged to a specific upgrade version.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1087125107
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeChecker.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * 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 java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.manager.EventCoordinator;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZKUtil;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class UpgradeChecker {
Review Comment:
Okay, checking on upgrade seems reasonable. I think maybe just adding a javadoc comment and updating names/references to it to make it clear it's a check for ZooKeeper prerequisite for the upgrade. Right now, it feels bolted on to upgrade, rather than integrated as part of the upgrade process. Polishing it up with location/naming/comments could help make it a bit more integrated into the upgrade process itself.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1088015001
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +140,14 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ if (cv < AccumuloDataVersion.ROOT_TABLET_META_CHANGES) {
+ throw new UnsupportedOperationException(
+ "Upgrading from a version before 2.1 is not supported. Upgrade to 2.1 before upgrading to 3.x");
+ }
+
Review Comment:
> Previously it was checked in each upgrader - the check here fails faster and provided a clearer message than if the upgradeder fail the version checks - failing with a message that upgrader for data version X is accurate, but may not translate to the user how data versions and Accumulo versions correlate.
>
> On the positive side - this provides a clear user message at the expense of needing to change the message if upgrades change - but that should be rare and I favored a better user experience.
Okay, but if we want to tie the message to the version number, then we should have some mapping of data version to the Accumulo version they appeared in, so we don't have to update this line. The number of locations where we have to made one or two character changes to keep all these messages accurate should be few, and centralized. Something like:
```suggestion
if (cv < oldestVersion) {
var oldRelease = dataVersionToReleaseName.get(oldestVersion);
throw new UnsupportedOperationException(
"Upgrading from a version before " + oldRelease + " is not supported. Upgrade to at least " + oldRelease + " before upgrading to " + Constants.VERSION);
}
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1090659932
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +140,14 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ if (cv < AccumuloDataVersion.ROOT_TABLET_META_CHANGES) {
+ throw new UnsupportedOperationException(
+ "Upgrading from a version before 2.1 is not supported. Upgrade to 2.1 before upgrading to 3.x");
+ }
+
Review Comment:
addressed in 70df59f3c7
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1087042300
##########
server/base/src/main/java/org/apache/accumulo/server/AccumuloDataVersion.java:
##########
@@ -47,18 +47,6 @@ public class AccumuloDataVersion {
*/
public static final int ROOT_TABLET_META_CHANGES = 10;
- /**
- * version (9) reflects changes to crypto that resulted in RFiles and WALs being serialized
- * differently in version 2.0.0. Also RFiles in 2.0.0 may have summary data.
- */
- public static final int CRYPTO_CHANGES = 9;
-
- /**
- * version (8) reflects changes to RFile index (ACCUMULO-1124) AND the change to WAL tracking in
- * ZK in version 1.8.0
- */
- public static final int SHORTEN_RFILE_KEYS = 8;
-
Review Comment:
The javadocs should be moved to the "historic data versions" section below.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeChecker.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * 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 java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.manager.EventCoordinator;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZKUtil;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class UpgradeChecker {
Review Comment:
What is this for? I wasn't expecting new code, since this task was to remove old code. This should have a javadoc to explain what it's for, at the least.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +140,14 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ if (cv < AccumuloDataVersion.ROOT_TABLET_META_CHANGES) {
+ throw new UnsupportedOperationException(
+ "Upgrading from a version before 2.1 is not supported. Upgrade to 2.1 before upgrading to 3.x");
+ }
+
Review Comment:
Isn't this already enforced elsewhere? This seems like a duplicate check that we'll need to maintain in more places. We should already be comparing against the `oldestSupported` field somewhere.
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,8 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = 8;
+ // as of 3.0 we will only support upgrades from 2.1
+ final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
Review Comment:
I'm not sure this comment expresses anything that the next line doesn't already express more clearly. As we move forward, the version names in this comment will get stale over time and the comment will need to be constantly updated. I'm not sure it's any more helpful than having the explicit assignment line right after.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1099350629
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +145,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = upgraders.entrySet().iterator().next().getKey();
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
+ + " data version (" + oldestVersion + ") is not supported. Upgrade to at least "
+ + oldRelease + " before upgrading to " + Constants.VERSION);
+ }
+
Review Comment:
Added in 2aea7e1f8d9 - will run an upgrade sequence and include the output for completeness later.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1099309126
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +145,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = upgraders.entrySet().iterator().next().getKey();
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
+ + " data version (" + oldestVersion + ") is not supported. Upgrade to at least "
+ + oldRelease + " before upgrading to " + Constants.VERSION);
+ }
+
Review Comment:
Sure - a quick look and the info I wanted is either private or in the javadoc.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1088008352
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,8 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = 8;
+ // as of 3.0 we will only support upgrades from 2.1
+ final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
Review Comment:
I don't mean the whole comment... most of the comment is fine... I just don't think it's useful to hard-code the version numbers into the comment, because that's not really adding anything that the rest of the comment doesn't already have... but by hard-coding these version names into the comment, it is making it more that we have to change when we move forward, in order to keep the comment current. I just think that's creating unnecessary work for ourselves, and in this case, I don't think it makes it any more clear than what was there before.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1090659368
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,8 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = 8;
+ // as of 3.0 we will only support upgrades from 2.1
+ final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
Review Comment:
Addressed in 70df59f3c7
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1098905837
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -107,10 +110,8 @@ public boolean isParentLevelUpgraded(KeyExtent extent) {
private int currentVersion;
// map of "current version" -> upgrader to next version.
- private final Map<Integer,
- Upgrader> upgraders = Map.of(AccumuloDataVersion.SHORTEN_RFILE_KEYS, new Upgrader8to9(),
- AccumuloDataVersion.CRYPTO_CHANGES, new Upgrader9to10(),
- AccumuloDataVersion.ROOT_TABLET_META_CHANGES, new Upgrader10to11());
+ private final Map<Integer,Upgrader> upgraders =
+ Map.of(ROOT_TABLET_META_CHANGES, new Upgrader10to11());
Review Comment:
Addressed in 7c8ec3ad63. I'll work a patch for 2.1.1 with only the ordering preserving and hard-coded value removal as a separate PR.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +143,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = ROOT_TABLET_META_CHANGES;
Review Comment:
Addressed in 7c8ec3ad63
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1098905837
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -107,10 +110,8 @@ public boolean isParentLevelUpgraded(KeyExtent extent) {
private int currentVersion;
// map of "current version" -> upgrader to next version.
- private final Map<Integer,
- Upgrader> upgraders = Map.of(AccumuloDataVersion.SHORTEN_RFILE_KEYS, new Upgrader8to9(),
- AccumuloDataVersion.CRYPTO_CHANGES, new Upgrader9to10(),
- AccumuloDataVersion.ROOT_TABLET_META_CHANGES, new Upgrader10to11());
+ private final Map<Integer,Upgrader> upgraders =
+ Map.of(ROOT_TABLET_META_CHANGES, new Upgrader10to11());
Review Comment:
Addressed in 7c8ec3ad63. I'll work a patch for 2.1 (1.10 does not have the class) with only the ordering preserving and hard-coded value removal as a separate PR.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1099159063
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +145,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = upgraders.entrySet().iterator().next().getKey();
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
+ + " data version (" + oldestVersion + ") is not supported. Upgrade to at least "
+ + oldRelease + " before upgrading to " + Constants.VERSION);
+ }
+
Review Comment:
I'll need to check - but pretty sure that during development I had a bad version set and the errors in the logs prompted me to put this in. Maybe because I and the new data version implements I was able to get this far, but I did reach the check.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1099306311
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +145,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = upgraders.entrySet().iterator().next().getKey();
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
+ + " data version (" + oldestVersion + ") is not supported. Upgrade to at least "
+ + oldRelease + " before upgrading to " + Constants.VERSION);
+ }
+
Review Comment:
I think we can get what you were going for by updating that current error message to include what you were trying to include here. I can take a look and add that change to this PR if you want.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1099251838
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +145,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = upgraders.entrySet().iterator().next().getKey();
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
+ + " data version (" + oldestVersion + ") is not supported. Upgrade to at least "
+ + oldRelease + " before upgrading to " + Constants.VERSION);
+ }
+
Review Comment:
You are correct. Going from 1.10 to 3.0-SNAPSHOT logs
```
2023-02-07T21:11:03,556 [start.Main] ERROR: Thread 'manager' died.
java.lang.IllegalStateException: This version of accumulo (3.0.0-SNAPSHOT) is not compatible with files stored using data version 8
at org.apache.accumulo.server.ServerContext.ensureDataVersionCompatible(ServerContext.java:302) ~[accumulo-server-base-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.server.ServerContext.init(ServerContext.java:371) ~[accumulo-server-base-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.server.AbstractServer.<init>(AbstractServer.java:50) ~[accumulo-server-base-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.manager.Manager.<init>(Manager.java:412) ~[accumulo-manager-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.manager.Manager.main(Manager.java:406) ~[accumulo-manager-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.manager.ManagerExecutable.execute(ManagerExecutable.java:45) ~[accumulo-manager-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.start.Main.lambda$execKeyword$0(Main.java:122) ~[accumulo-start-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at java.lang.Thread.run(Thread.java:829) ~[?:?]
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1100200971
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +145,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = upgraders.entrySet().iterator().next().getKey();
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
+ + " data version (" + oldestVersion + ") is not supported. Upgrade to at least "
+ + oldRelease + " before upgrading to " + Constants.VERSION);
+ }
+
Review Comment:
With the changes - the error message now reads:
```2023-02-08T14:15:02,313 [start.Main] ERROR: Thread 'manager' died.
java.lang.IllegalStateException: This version of accumulo (3.0.0-SNAPSHOT) is not compatible with files stored using data version 8. Please upgrade from 2.1.0 or later.
at org.apache.accumulo.server.ServerContext.ensureDataVersionCompatible(ServerContext.java:304) ~[accumulo-server-base-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.server.ServerContext.init(ServerContext.java:373) ~[accumulo-server-base-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.server.AbstractServer.<init>(AbstractServer.java:50) ~[accumulo-server-base-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.manager.Manager.<init>(Manager.java:412) ~[accumulo-manager-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.manager.Manager.main(Manager.java:406) ~[accumulo-manager-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.manager.ManagerExecutable.execute(ManagerExecutable.java:45) ~[accumulo-manager-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.start.Main.lambda$execKeyword$0(Main.java:122) ~[accumulo-start-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3160: remove upgrade code for versions before 2.1
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#issuecomment-1398724375
This will resolve https://github.com/apache/accumulo/issues/3098
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1098905837
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -107,10 +110,8 @@ public boolean isParentLevelUpgraded(KeyExtent extent) {
private int currentVersion;
// map of "current version" -> upgrader to next version.
- private final Map<Integer,
- Upgrader> upgraders = Map.of(AccumuloDataVersion.SHORTEN_RFILE_KEYS, new Upgrader8to9(),
- AccumuloDataVersion.CRYPTO_CHANGES, new Upgrader9to10(),
- AccumuloDataVersion.ROOT_TABLET_META_CHANGES, new Upgrader10to11());
+ private final Map<Integer,Upgrader> upgraders =
+ Map.of(ROOT_TABLET_META_CHANGES, new Upgrader10to11());
Review Comment:
Addressed in 7c8ec3ad63. I'll work a patch for 1.10 with only the ordering preserving and hard-coded value removal as a separate PR.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1087294704
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeChecker.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * 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 java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.manager.EventCoordinator;
+import org.apache.accumulo.server.AccumuloDataVersion;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZKUtil;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class UpgradeChecker {
Review Comment:
Addressed in a58a977761 - Renamed the class to PreUpgradeValidation and added javadoc
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1087252944
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,8 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = 8;
+ // as of 3.0 we will only support upgrades from 2.1
+ final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
Review Comment:
The comment was pre-existing and I found it helpful - it stopped me in my tracks to make sure that removing the previous versions was what was intended. Removing upgrade versions should be rare and having it explicitly stated here served as a good reminder / check.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1087286437
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +140,14 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ if (cv < AccumuloDataVersion.ROOT_TABLET_META_CHANGES) {
+ throw new UnsupportedOperationException(
+ "Upgrading from a version before 2.1 is not supported. Upgrade to 2.1 before upgrading to 3.x");
+ }
+
Review Comment:
Previously it was checked in each upgrader - the check here fails faster and provided a clearer message than if the upgradeder fail the version checks - failing with a message that upgrader for data version X is accurate, but may not translate to the user how data versions and Accumulo versions correlate.
On the positive side - this provides a clear user message at the expense of needing to change the message if upgrades change - but that should be rare and I favored a better user experience.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1088015001
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +140,14 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ if (cv < AccumuloDataVersion.ROOT_TABLET_META_CHANGES) {
+ throw new UnsupportedOperationException(
+ "Upgrading from a version before 2.1 is not supported. Upgrade to 2.1 before upgrading to 3.x");
+ }
+
Review Comment:
> Previously it was checked in each upgrader - the check here fails faster and provided a clearer message than if the upgradeder fail the version checks - failing with a message that upgrader for data version X is accurate, but may not translate to the user how data versions and Accumulo versions correlate.
>
> On the positive side - this provides a clear user message at the expense of needing to change the message if upgrades change - but that should be rare and I favored a better user experience.
Okay, but if we want to tie the message to the version number, then we should have some mapping of data version to the Accumulo version they appeared in, so we don't have to update this line. The number of locations where we have to made one or two character changes to keep all these messages accurate should be few, and centralized. Something like:
```suggestion
if (cv < oldestVersion) {
String oldRelease = dataVersionToReleaseName.get(oldestVersion);
throw new UnsupportedOperationException(
"Upgrading from a version before " + oldRelease + " is not supported. Upgrade to at least " + oldRelease + " before upgrading to " + Constants.VERSION);
}
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1099274556
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +145,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = upgraders.entrySet().iterator().next().getKey();
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
+ + " data version (" + oldestVersion + ") is not supported. Upgrade to at least "
+ + oldRelease + " before upgrading to " + Constants.VERSION);
+ }
+
Review Comment:
@ctubbsii would this be okay? Seems long and not adding much - might just be better to include something in the release notes or in the upgrading section of the docs.
```
java.lang.IllegalStateException: This version of accumulo (3.0.0-SNAPSHOT) is not compatible with files stored using data version 8 see org.apache.accumulo.server.AccumuloDataVersion javadoc for historic version mapping
at org.apache.accumulo.server.ServerContext.ensureDataVersionCompatible(ServerContext.java:302) ~[accumulo-server-base-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.server.ServerContext.init(ServerContext.java:372) ~[accumulo-server-base-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.server.AbstractServer.<init>(AbstractServer.java:50) ~[accumulo-server-base-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.manager.Manager.<init>(Manager.java:412) ~[accumulo-manager-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.manager.Manager.main(Manager.java:406) ~[accumulo-manager-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.manager.ManagerExecutable.execute(ManagerExecutable.java:45) ~[accumulo-manager-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at org.apache.accumulo.start.Main.lambda$execKeyword$0(Main.java:122) ~[accumulo-start-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
at java.lang.Thread.run(Thread.java:829) ~[?:?]
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1099314206
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +145,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = upgraders.entrySet().iterator().next().getKey();
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
+ + " data version (" + oldestVersion + ") is not supported. Upgrade to at least "
+ + oldRelease + " before upgrading to " + Constants.VERSION);
+ }
+
Review Comment:
a5da59abc76 removes the test and leaves the message alone.
The improvement that I was shooting for would provide guidance in the manage log when the upgrade fails. The upgrade is using data version and while that is tied to release version, that connection is buried in our code. Users may not be familiar with "data version", where it is set / coming from and how that even relates to a release.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1099314206
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +145,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = upgraders.entrySet().iterator().next().getKey();
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
+ + " data version (" + oldestVersion + ") is not supported. Upgrade to at least "
+ + oldRelease + " before upgrading to " + Constants.VERSION);
+ }
+
Review Comment:
a5da59abc76 removes the test and leaves the message alone.
The improvement that I was shooting for would provide guidance in the manager log when the upgrade fails. The upgrade is using data version and while that is tied to release version, that connection is buried in our code. Users may not be familiar with "data version", where it is set / coming from and how that even relates to a release.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1097973247
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -107,10 +110,8 @@ public boolean isParentLevelUpgraded(KeyExtent extent) {
private int currentVersion;
// map of "current version" -> upgrader to next version.
- private final Map<Integer,
- Upgrader> upgraders = Map.of(AccumuloDataVersion.SHORTEN_RFILE_KEYS, new Upgrader8to9(),
- AccumuloDataVersion.CRYPTO_CHANGES, new Upgrader9to10(),
- AccumuloDataVersion.ROOT_TABLET_META_CHANGES, new Upgrader10to11());
+ private final Map<Integer,Upgrader> upgraders =
+ Map.of(ROOT_TABLET_META_CHANGES, new Upgrader10to11());
Review Comment:
This might be a problem with our existing upgrader code, but `Map.of` does not guarantee ordering. Taken from [the javadoc for unmodifiable maps](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Map.html#unmodifiable): `The iteration order of mappings is unspecified and is subject to change`
In this case, it's a singleton map... but in previous versions, and in the future, it may not be. We should use a map that preserves the order, because these upgraders need to be executed in order, I believe.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +143,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = ROOT_TABLET_META_CHANGES;
Review Comment:
Instead of having another place where this is hard-coded, can just get the first upgrader mapping key:
```suggestion
int oldestVersion = upgraders.iterator().next().getKey();
```
However, this requires a predictable ordering of the upgraders... which we might already be relying on, and which might be an existing problem. See my other comment on this.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +143,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = ROOT_TABLET_META_CHANGES;
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version before " + oldRelease
Review Comment:
I think "less than" makes more sense here than "before", since "before" could mean chronologically, and we might release a bugfix for an earlier release line after the `.0` version we're expecting. But that bugfix's version would still be "less than" the `.0` version release.
```suggestion
throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
```
##########
server/base/src/main/java/org/apache/accumulo/server/AccumuloDataVersion.java:
##########
@@ -47,22 +47,12 @@ public class AccumuloDataVersion {
*/
public static final int ROOT_TABLET_META_CHANGES = 10;
- /**
- * version (9) reflects changes to crypto that resulted in RFiles and WALs being serialized
- * differently in version 2.0.0. Also RFiles in 2.0.0 may have summary data.
- */
- public static final int CRYPTO_CHANGES = 9;
-
- /**
- * version (8) reflects changes to RFile index (ACCUMULO-1124) AND the change to WAL tracking in
- * ZK in version 1.8.0
- */
- public static final int SHORTEN_RFILE_KEYS = 8;
-
/**
* Historic data versions
*
* <ul>
+ * <li>version (9) RFiles and wal crypto serialization changes. RFile summary data in 2.0.0</li>
+ * <li>version (8) RFile index (ACCUMULO-1124) and wal tracking in ZK</li>
Review Comment:
```suggestion
* <li>version (8) RFile index (ACCUMULO-1124) and wal tracking in ZK in 1.8.0</li>
```
##########
server/base/src/main/java/org/apache/accumulo/server/AccumuloDataVersion.java:
##########
@@ -81,6 +71,28 @@ public static int get() {
return CURRENT_VERSION;
}
- public static final Set<Integer> CAN_RUN =
- Set.of(SHORTEN_RFILE_KEYS, CRYPTO_CHANGES, ROOT_TABLET_META_CHANGES, CURRENT_VERSION);
+ public static final Set<Integer> CAN_RUN = Set.of(ROOT_TABLET_META_CHANGES, CURRENT_VERSION);
+
+ /**
+ * Get the stored, current working version.
+ *
+ * @param context the server context
+ * @return the stored data version
+ */
+ public static int getCurrentVersion(ServerContext context) {
+ int cv =
+ context.getServerDirs().getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
+ ServerContext.ensureDataVersionCompatible(cv);
+ return cv;
+ }
+
+ public static String dataVersionToReleaseName(final int version) {
+ switch (version) {
+ case ROOT_TABLET_META_CHANGES:
+ return "2.1";
+ case REMOVE_DEPRECATIONS_FOR_VERSION_3:
+ return "3.0";
Review Comment:
It's highly unlikely, but possible that a data version could change in a bugfix release. Also, because data version can span multiple minor versions, it may not make sense to only show the minor release here, as that could imply it only applies to versions matching `a.b.*`, when it might also apply to version `a.b+1.*` as well. So, instead, let's use the full name of the version of the first appearance of the change.
```suggestion
return "2.1.0";
case REMOVE_DEPRECATIONS_FOR_VERSION_3:
return "3.0.0";
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1099062325
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -142,11 +145,17 @@ public synchronized void upgradeZookeeper(ServerContext context,
"Not currently in a suitable state to do zookeeper upgrade %s", status);
try {
- int cv = context.getServerDirs()
- .getAccumuloPersistentVersion(context.getVolumeManager().getFirst());
- ServerContext.ensureDataVersionCompatible(cv);
+ int cv = AccumuloDataVersion.getCurrentVersion(context);
this.currentVersion = cv;
+ int oldestVersion = upgraders.entrySet().iterator().next().getKey();
+ if (cv < oldestVersion) {
+ String oldRelease = dataVersionToReleaseName(oldestVersion);
+ throw new UnsupportedOperationException("Upgrading from a version less than " + oldRelease
+ + " data version (" + oldestVersion + ") is not supported. Upgrade to at least "
+ + oldRelease + " before upgrading to " + Constants.VERSION);
+ }
+
Review Comment:
In thinking about this some more, I don't think this added message is ever going to work... this can only execute after the `UpgradeCoordinator` is instantiated... which won't happen until after the `Manager` is constructed... but the `Manager` won't successfully get constructed because `ServerContext.init` will fail long before this, when it runs `ServerContext.ensureDataVersionCompatible`, that checks `AccumuloDataVersion.CAN_RUN`.
I think if you want to have this more informative message, you're going to need to edit `ServerContext.ensureDataVersionCompatible` instead and this added check can just be removed, as it's not doing anything useful here.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3160: remove upgrade code for versions before 2.1
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#issuecomment-1382332262
Pushed this as a draft for comments - still want to double check for unnecessary tests and code that may no longer be references, but this does remove most of the previous upgrade code as is.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3160: remove upgrade code for versions before 2.1
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3160:
URL: https://github.com/apache/accumulo/pull/3160#discussion_r1087295011
##########
server/base/src/main/java/org/apache/accumulo/server/AccumuloDataVersion.java:
##########
@@ -47,18 +47,6 @@ public class AccumuloDataVersion {
*/
public static final int ROOT_TABLET_META_CHANGES = 10;
- /**
- * version (9) reflects changes to crypto that resulted in RFiles and WALs being serialized
- * differently in version 2.0.0. Also RFiles in 2.0.0 may have summary data.
- */
- public static final int CRYPTO_CHANGES = 9;
-
- /**
- * version (8) reflects changes to RFile index (ACCUMULO-1124) AND the change to WAL tracking in
- * ZK in version 1.8.0
- */
- public static final int SHORTEN_RFILE_KEYS = 8;
-
Review Comment:
Addressed in a58a977761,
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org