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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
