You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2025/03/13 19:32:25 UTC
[PR] Fixed UpgradeCoordinator, UpgradeProgressTracker, and Upgrader12to13 [accumulo]
dlmarion opened a new pull request, #5402:
URL: https://github.com/apache/accumulo/pull/5402
Updated the UpgradeCoordinator to fix what is essentially an off by one bug. The version of the instance and the key in the upgraders map did not match the way the code was processing the versions.
Updated the UpgradeProgressTracker with additional checks.
Updated Upgrader12to13 methods to be invoked during the appropriate phase and to be re-entrant.
--
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
Re: [PR] Fixed UpgradeCoordinator, UpgradeProgressTracker, and Upgrader12to13 [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5402:
URL: https://github.com/apache/accumulo/pull/5402#discussion_r1998615033
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -156,6 +162,15 @@ public synchronized UpgradeProgress getProgress() {
}
public synchronized void upgradeComplete() {
+ checkArgument(progress.getZooKeeperVersion() == AccumuloDataVersion.get(),
Review Comment:
Fixed in bfab98d
--
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
Re: [PR] Fixed UpgradeCoordinator, UpgradeProgressTracker, and Upgrader12to13 [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5402:
URL: https://github.com/apache/accumulo/pull/5402#discussion_r1994311817
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -207,7 +207,11 @@ public synchronized void upgradeZookeeper(EventCoordinator eventCoordinator) {
Objects.requireNonNull(upgrader,
"upgrade ZooKeeper: failed to find upgrader for version " + upgradeVersion);
upgrader.upgradeZookeeper(context);
- progressTracker.updateZooKeeperVersion(upgradeVersion);
+ // The current version and the versions in the upgraders map are equal. For example,
+ // if the current version is 10, then the code above will get the Upgrader for version
+ // 10 from the upgraders map. It will run the Upgrader10to11 code, which means that the
+ // current version of ZooKeeper after running the upgrade is 11.
+ progressTracker.updateZooKeeperVersion(upgradeVersion + 1);
Review Comment:
Upgrader10to11 was not being run, it was starting with Upgrader11to12. The progress tracker gets initialized with the current version (10 in this case). The upgradeVersion is initialized at the current version (10, in this case). The `>=` check on line 198 was causing it to skip Upgrader10to11. I couldn't really change the upgradeVersion and how it works because the map of Upgraders is keyed on the source versions (10 is the key in the upgraders map).
--
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
Re: [PR] Fixed UpgradeCoordinator, UpgradeProgressTracker, and Upgrader12to13 [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5402:
URL: https://github.com/apache/accumulo/pull/5402#discussion_r1999464304
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader12to13.java:
##########
@@ -129,8 +129,20 @@ private static void addCompactionsNode(ServerContext context) {
}
private void createFateTable(ServerContext context) {
- ZooKeeperInitializer zkInit = new ZooKeeperInitializer();
- zkInit.initFateTableState(context);
+
+ if (context.tableOperations().exists(AccumuloTable.FATE.tableName())) {
+ LOG.info("Fate table already exists");
+ return;
+ }
+
+ try {
+ ZooKeeperInitializer zkInit = new ZooKeeperInitializer();
+ zkInit.initFateTableState(context);
+ } catch (Exception e) {
+ if (e.getCause() instanceof KeeperException.NodeExistsException) {
+ LOG.debug("Fate table node already exists in ZooKeeper");
+ }
+ }
Review Comment:
Fixed in #5409
--
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
Re: [PR] Fixed UpgradeCoordinator, UpgradeProgressTracker, and Upgrader12to13 [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5402:
URL: https://github.com/apache/accumulo/pull/5402#discussion_r1999447657
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader12to13.java:
##########
@@ -129,8 +129,20 @@ private static void addCompactionsNode(ServerContext context) {
}
private void createFateTable(ServerContext context) {
- ZooKeeperInitializer zkInit = new ZooKeeperInitializer();
- zkInit.initFateTableState(context);
+
+ if (context.tableOperations().exists(AccumuloTable.FATE.tableName())) {
+ LOG.info("Fate table already exists");
+ return;
+ }
+
+ try {
+ ZooKeeperInitializer zkInit = new ZooKeeperInitializer();
+ zkInit.initFateTableState(context);
+ } catch (Exception e) {
+ if (e.getCause() instanceof KeeperException.NodeExistsException) {
+ LOG.debug("Fate table node already exists in ZooKeeper");
+ }
+ }
Review Comment:
Great catch!
--
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
Re: [PR] Fixed UpgradeCoordinator, UpgradeProgressTracker, and Upgrader12to13 [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #5402:
URL: https://github.com/apache/accumulo/pull/5402
--
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
Re: [PR] Fixed UpgradeCoordinator, UpgradeProgressTracker, and Upgrader12to13 [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5402:
URL: https://github.com/apache/accumulo/pull/5402#issuecomment-2722491645
I got a successful 2.1 to 4.0 upgrade with these changes (not that the 4.0 upgrade code is complete, but the process finished). I stopped and started multiple times, testing the progress tracking changes that were made in an earlier change.
--
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
Re: [PR] Fixed UpgradeCoordinator, UpgradeProgressTracker, and Upgrader12to13 [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5402:
URL: https://github.com/apache/accumulo/pull/5402#discussion_r1994441572
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -207,7 +207,11 @@ public synchronized void upgradeZookeeper(EventCoordinator eventCoordinator) {
Objects.requireNonNull(upgrader,
"upgrade ZooKeeper: failed to find upgrader for version " + upgradeVersion);
upgrader.upgradeZookeeper(context);
- progressTracker.updateZooKeeperVersion(upgradeVersion);
+ // The current version and the versions in the upgraders map are equal. For example,
+ // if the current version is 10, then the code above will get the Upgrader for version
+ // 10 from the upgraders map. It will run the Upgrader10to11 code, which means that the
+ // current version of ZooKeeper after running the upgrade is 11.
+ progressTracker.updateZooKeeperVersion(upgradeVersion + 1);
Review Comment:
OK I see, I was thinking this would only cause a problem after a restart in the middle upgrade.
--
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
Re: [PR] Fixed UpgradeCoordinator, UpgradeProgressTracker, and Upgrader12to13 [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5402:
URL: https://github.com/apache/accumulo/pull/5402#discussion_r1994305544
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeCoordinator.java:
##########
@@ -207,7 +207,11 @@ public synchronized void upgradeZookeeper(EventCoordinator eventCoordinator) {
Objects.requireNonNull(upgrader,
"upgrade ZooKeeper: failed to find upgrader for version " + upgradeVersion);
upgrader.upgradeZookeeper(context);
- progressTracker.updateZooKeeperVersion(upgradeVersion);
+ // The current version and the versions in the upgraders map are equal. For example,
+ // if the current version is 10, then the code above will get the Upgrader for version
+ // 10 from the upgraders map. It will run the Upgrader10to11 code, which means that the
+ // current version of ZooKeeper after running the upgrade is 11.
+ progressTracker.updateZooKeeperVersion(upgradeVersion + 1);
Review Comment:
How did you find this bug? Curious if it caused some sort of failure.
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgradeProgressTracker.java:
##########
@@ -156,6 +162,15 @@ public synchronized UpgradeProgress getProgress() {
}
public synchronized void upgradeComplete() {
+ checkArgument(progress.getZooKeeperVersion() == AccumuloDataVersion.get(),
Review Comment:
could call checkState here because its not checking an argument passed into the function.
--
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
Re: [PR] Fixed UpgradeCoordinator, UpgradeProgressTracker, and Upgrader12to13 [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5402:
URL: https://github.com/apache/accumulo/pull/5402#discussion_r1999440815
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader12to13.java:
##########
@@ -129,8 +129,20 @@ private static void addCompactionsNode(ServerContext context) {
}
private void createFateTable(ServerContext context) {
- ZooKeeperInitializer zkInit = new ZooKeeperInitializer();
- zkInit.initFateTableState(context);
+
+ if (context.tableOperations().exists(AccumuloTable.FATE.tableName())) {
+ LOG.info("Fate table already exists");
+ return;
+ }
+
+ try {
+ ZooKeeperInitializer zkInit = new ZooKeeperInitializer();
+ zkInit.initFateTableState(context);
+ } catch (Exception e) {
+ if (e.getCause() instanceof KeeperException.NodeExistsException) {
+ LOG.debug("Fate table node already exists in ZooKeeper");
+ }
+ }
Review Comment:
@dlmarion This is equivalent to:
```java
} catch (KeeperException.NodeExistsException e) {
LOG.debug("Fate table node already exists in ZooKeeper");
} catch (Exception e) {
// completely swallow and ignore
}
```
Are you sure this is the intended behavior? It looks very sketchy to me.
--
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