This is an automated email from the ASF dual-hosted git repository.
ctubbsii pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/2.1 by this push:
new fffed51104 Verify upgrade is complete before starting fate (#3717)
fffed51104 is described below
commit fffed511045a3e49635bda30d1cd5153fa345a9a
Author: Christopher Tubbs <[email protected]>
AuthorDate: Sun Jul 28 21:08:25 2024 -0400
Verify upgrade is complete before starting fate (#3717)
* Simplify checking upgrade and fate status during state changes by
changing the implementation of `isUpgrading()` to use the
upgradeCoordinator, explicitly checking if upgrades are complete
before starting fate (related to #2990 and fixes #3716), and only
running the upgrade tasks on state changes if the upgrade coordinator
isn't done with upgrades
* Throw an exception if an illegal state change is detected, rather than
merely log an error
* Also fix some code quality issues by removing redundant check for
`oldState != newState`, using a switch statement for newState instead,
and using enum equality check as `==` instead of `.equals()`
This fixes #3716
---
.../java/org/apache/accumulo/manager/Manager.java | 81 ++++++++++------------
1 file changed, 38 insertions(+), 43 deletions(-)
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java
b/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java
index a72485af01..a554b2d818 100644
--- a/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java
+++ b/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java
@@ -220,7 +220,6 @@ public class Manager extends AbstractServer
final ServerBulkImportStatus bulkImportStatus = new ServerBulkImportStatus();
private final AtomicBoolean managerInitialized = new AtomicBoolean(false);
- private final AtomicBoolean managerUpgrading = new AtomicBoolean(false);
private final long timeToCacheRecoveryWalExistence;
@Override
@@ -279,38 +278,40 @@ public class Manager extends AbstractServer
/* UNLOAD_ROOT_TABLET */ {O, O, O, X, X, X, X},
/* STOP */ {O, O, O, O, O, X, X}};
//@formatter:on
- synchronized void setManagerState(ManagerState newState) {
- if (state.equals(newState)) {
+ synchronized void setManagerState(final ManagerState newState) {
+ if (state == newState) {
return;
}
if (!transitionOK[state.ordinal()][newState.ordinal()]) {
- log.error("Programmer error: manager should not transition from {} to
{}", state, newState);
+ throw new IllegalStateException(String.format(
+ "Programmer error: manager should not transition from %s to %s",
state, newState));
}
- ManagerState oldState = state;
+ final ManagerState oldState = state;
state = newState;
nextEvent.event("State changed from %s to %s", oldState, newState);
- if (newState == ManagerState.STOP) {
- // Give the server a little time before shutdown so the client
- // thread requesting the stop can return
- ScheduledFuture<?> future =
getContext().getScheduledExecutor().scheduleWithFixedDelay(() -> {
- // This frees the main thread and will cause the manager to exit
- clientService.stop();
- Manager.this.nextEvent.event("stopped event loop");
- }, 100L, 1000L, MILLISECONDS);
- ThreadPools.watchNonCriticalScheduledTask(future);
- }
-
- if (oldState != newState && (newState == ManagerState.HAVE_LOCK)) {
- upgradeCoordinator.upgradeZookeeper(getContext(), nextEvent);
- }
-
- if (oldState != newState && (newState == ManagerState.NORMAL)) {
- if (fateRef.get() != null) {
- throw new IllegalStateException("Access to Fate should not have been"
- + " initialized prior to the Manager finishing upgrades. Please
save"
- + " all logs and file a bug.");
- }
- upgradeMetadataFuture = upgradeCoordinator.upgradeMetadata(getContext(),
nextEvent);
+ switch (newState) {
+ case STOP:
+ // Give the server a little time before shutdown so the client
+ // thread requesting the stop can return
+ final var future =
getContext().getScheduledExecutor().scheduleWithFixedDelay(() -> {
+ // This frees the main thread and will cause the manager to exit
+ clientService.stop();
+ Manager.this.nextEvent.event("stopped event loop");
+ }, 100L, 1000L, MILLISECONDS);
+ ThreadPools.watchNonCriticalScheduledTask(future);
+ break;
+ case HAVE_LOCK:
+ if (isUpgrading()) {
+ upgradeCoordinator.upgradeZookeeper(getContext(), nextEvent);
+ }
+ break;
+ case NORMAL:
+ if (isUpgrading()) {
+ upgradeMetadataFuture =
upgradeCoordinator.upgradeMetadata(getContext(), nextEvent);
+ }
+ break;
+ default:
+ break;
}
}
@@ -854,10 +855,8 @@ public class Manager extends AbstractServer
setManagerState(ManagerState.NORMAL);
break;
case SAFE_MODE:
- if (getManagerState() == ManagerState.NORMAL) {
- setManagerState(ManagerState.SAFE_MODE);
- }
- if (getManagerState() == ManagerState.HAVE_LOCK) {
+ if (getManagerState() == ManagerState.NORMAL
+ || getManagerState() == ManagerState.HAVE_LOCK) {
setManagerState(ManagerState.SAFE_MODE);
}
break;
@@ -1241,12 +1240,6 @@ public class Manager extends AbstractServer
throw new IllegalStateException("Exception getting manager lock", e);
}
- // If UpgradeStatus is not at complete by this moment, then things are
currently
- // upgrading.
- if (upgradeCoordinator.getStatus() !=
UpgradeCoordinator.UpgradeStatus.COMPLETE) {
- managerUpgrading.set(true);
- }
-
MetricsInfo metricsInfo = getContext().getMetricsInfo();
metricsInfo.addServiceTags(getApplicationName(), sa.getAddress());
@@ -1329,15 +1322,17 @@ public class Manager extends AbstractServer
// Once we are sure the upgrade is complete, we can safely allow fate use.
try {
// wait for metadata upgrade running in background to complete
- if (null != upgradeMetadataFuture) {
+ if (upgradeMetadataFuture != null) {
upgradeMetadataFuture.get();
}
- // Everything is fully upgraded by this point.
- managerUpgrading.set(false);
} catch (ExecutionException | InterruptedException e) {
throw new IllegalStateException("Metadata upgrade failed", e);
}
+ // Everything should be fully upgraded by this point, but check before
starting fate
+ if (isUpgrading()) {
+ throw new IllegalStateException("Upgrade coordinator is unexpectedly not
complete");
+ }
try {
final AgeOffStore<Manager> store = new AgeOffStore<>(
new org.apache.accumulo.core.fate.ZooStore<>(getZooKeeperRoot() +
Constants.ZFATE,
@@ -1850,11 +1845,11 @@ public class Manager extends AbstractServer
}
public void assignedTablet(KeyExtent extent) {
- if (extent.isMeta() &&
getManagerState().equals(ManagerState.UNLOAD_ROOT_TABLET)) {
+ if (extent.isMeta() && getManagerState() ==
ManagerState.UNLOAD_ROOT_TABLET) {
setManagerState(ManagerState.UNLOAD_METADATA_TABLETS);
}
// probably too late, but try anyhow
- if (extent.isRootTablet() && getManagerState().equals(ManagerState.STOP)) {
+ if (extent.isRootTablet() && getManagerState() == ManagerState.STOP) {
setManagerState(ManagerState.UNLOAD_ROOT_TABLET);
}
}
@@ -1956,7 +1951,7 @@ public class Manager extends AbstractServer
@Override
public boolean isUpgrading() {
- return managerUpgrading.get();
+ return upgradeCoordinator.getStatus() !=
UpgradeCoordinator.UpgradeStatus.COMPLETE;
}
void initializeBalancer() {
