ctubbsii commented on code in PR #5383:
URL: https://github.com/apache/accumulo/pull/5383#discussion_r2036260228
##########
server/base/src/main/java/org/apache/accumulo/server/util/UpgradeUtil.java:
##########
@@ -48,6 +59,17 @@ static class Opts extends ConfigOpts {
description = "prepare an older version instance for an upgrade to a
newer non-bugfix release."
+ " This command should be run using the older version of software
after the instance is shut down.")
boolean prepare = false;
+
+ @Parameter(names = "--start",
+ description = "Start an upgrade of an Accumulo instance. This will
check that 'accumulo upgrade --prepare'"
+ + " was run on the instance after it was shut down, perform
pre-upgrade validation, and perform any"
+ + " upgrade steps that need to occur before the Manager is
started. Finally it creates a mandatory"
Review Comment:
```suggestion
+ " upgrade steps that need to occur before the Manager is
started. Finally, it creates a mandatory"
```
##########
server/base/src/main/java/org/apache/accumulo/server/util/UpgradeUtil.java:
##########
@@ -57,79 +79,227 @@ public String keyword() {
@Override
public String description() {
- return "utility used to perform various upgrade steps for an Accumulo
instance";
+ return "utility used to perform various upgrade steps for an Accumulo
instance. The 'prepare'"
+ + " step is intended to be run using the old version of software after
an instance has"
+ + " been shut down. The 'start' step is intended to be run on the
instance with the new"
+ + " version of software. Server processes should fail to start after
the 'prepare' step"
+ + " has been run due to the existence of a node in ZooKeeper. When the
'start' step"
+ + " completes successfully it will remove this node allowing the user
to start the"
+ + " Manager to complete the instance upgrade process.";
+ }
+
+ private void prepare(final ServerContext context) {
+
+ final int persistentVersion =
AccumuloDataVersion.getCurrentVersion(context);
+ final int thisVersion = AccumuloDataVersion.get();
+ if (persistentVersion != thisVersion) {
+ throw new IllegalStateException("It looks like you are running 'prepare'
with "
+ + "a different version of software than what the instance was
running with."
+ + " The 'prepare' command is intended to be run after an instance is
shutdown"
+ + " with the same version of software before trying to upgrade.");
+ }
+
+ final ZooSession zs = context.getZooSession();
+ final ZooReaderWriter zoo = zs.asReaderWriter();
+
+ try {
+ if (zoo.exists(ZPREPARE_FOR_UPGRADE)) {
+ zoo.delete(ZPREPARE_FOR_UPGRADE);
+ }
+ } catch (KeeperException | InterruptedException e) {
+ throw new IllegalStateException("Error creating or checking for " +
ZPREPARE_FOR_UPGRADE
+ + " node in zookeeper: " + e.getMessage(), e);
Review Comment:
```suggestion
throw new IllegalStateException("Error creating or checking for " +
ZPREPARE_FOR_UPGRADE
+ " node in zookeeper", e);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -328,8 +328,8 @@ synchronized void setManagerState(final ManagerState
newState) {
break;
case HAVE_LOCK:
if (isUpgrading()) {
+ upgradeCoordinator.continueUpgrade();
upgradeCoordinator.preUpgradeValidation();
Review Comment:
I don't think the preUpgradeValidation needs to run inside the manager at
all. Its function is superseded by these changes that initiates the upgrade
process before the manager is run. Once the upgrade has started, it doesn't
have any value.
##########
server/base/src/main/java/org/apache/accumulo/server/util/UpgradeUtil.java:
##########
@@ -48,6 +59,17 @@ static class Opts extends ConfigOpts {
description = "prepare an older version instance for an upgrade to a
newer non-bugfix release."
+ " This command should be run using the older version of software
after the instance is shut down.")
boolean prepare = false;
+
+ @Parameter(names = "--start",
+ description = "Start an upgrade of an Accumulo instance. This will
check that 'accumulo upgrade --prepare'"
+ + " was run on the instance after it was shut down, perform
pre-upgrade validation, and perform any"
+ + " upgrade steps that need to occur before the Manager is
started. Finally it creates a mandatory"
Review Comment:
Just so I understand, if this marker does not exist in ZK, and the current
data version is not the version of the software running, the Manager will not
start, correct?
(Also, for that check, will the manager check all volumes for the version?)
##########
server/base/src/main/java/org/apache/accumulo/server/util/UpgradeUtil.java:
##########
@@ -57,79 +79,227 @@ public String keyword() {
@Override
public String description() {
- return "utility used to perform various upgrade steps for an Accumulo
instance";
+ return "utility used to perform various upgrade steps for an Accumulo
instance. The 'prepare'"
+ + " step is intended to be run using the old version of software after
an instance has"
+ + " been shut down. The 'start' step is intended to be run on the
instance with the new"
+ + " version of software. Server processes should fail to start after
the 'prepare' step"
+ + " has been run due to the existence of a node in ZooKeeper. When the
'start' step"
+ + " completes successfully it will remove this node allowing the user
to start the"
+ + " Manager to complete the instance upgrade process.";
+ }
+
+ private void prepare(final ServerContext context) {
+
+ final int persistentVersion =
AccumuloDataVersion.getCurrentVersion(context);
+ final int thisVersion = AccumuloDataVersion.get();
+ if (persistentVersion != thisVersion) {
+ throw new IllegalStateException("It looks like you are running 'prepare'
with "
+ + "a different version of software than what the instance was
running with."
+ + " The 'prepare' command is intended to be run after an instance is
shutdown"
+ + " with the same version of software before trying to upgrade.");
+ }
+
+ final ZooSession zs = context.getZooSession();
+ final ZooReaderWriter zoo = zs.asReaderWriter();
+
+ try {
+ if (zoo.exists(ZPREPARE_FOR_UPGRADE)) {
+ zoo.delete(ZPREPARE_FOR_UPGRADE);
+ }
Review Comment:
This has a race condition. It's probably better to just call delete and
ignore the NoNodeException.
--
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-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
