You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/04/21 17:00:20 UTC
[GitHub] [ozone] errose28 commented on a change in pull request #2160: HDDS-5108. Attempt to remove state from *UpgradeFinalizer classes.
errose28 commented on a change in pull request #2160:
URL: https://github.com/apache/ozone/pull/2160#discussion_r616988686
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/DefaultUpgradeFinalizationExecutor.java
##########
@@ -30,48 +31,37 @@
* Unit/Integration tests can override this to provide error injected version
* of this class.
*/
-
-@SuppressWarnings("checkstyle:VisibilityModifier")
-public class DefaultUpgradeFinalizationExecutor {
+public class DefaultUpgradeFinalizationExecutor<T> {
static final Logger LOG =
LoggerFactory.getLogger(DefaultUpgradeFinalizationExecutor.class);
public DefaultUpgradeFinalizationExecutor() {
}
- public Void execute(Storage storageConfig,
- BasicUpgradeFinalizer basicUpgradeFinalizer)
- throws Exception {
+ public Void execute(T component, BasicUpgradeFinalizer finalizer)
+ throws IOException {
try {
- basicUpgradeFinalizer.emitStartingMsg();
- basicUpgradeFinalizer.getVersionManager()
+ finalizer.emitStartingMsg();
+ finalizer.getVersionManager()
.setUpgradeState(FINALIZATION_IN_PROGRESS);
- /*
- * Before we can call finalize the feature, we need to make sure that
- * all existing pipelines are closed and pipeline Manger would freeze
- * all new pipeline creation.
- */
- if(!basicUpgradeFinalizer.preFinalizeUpgrade()) {
- return null;
- }
+ finalizer.preFinalizeUpgrade(component);
- basicUpgradeFinalizer.finalizeUpgrade(storageConfig);
+ finalizer.finalizeUpgrade(component);
- basicUpgradeFinalizer.postFinalizeUpgrade();
+ finalizer.postFinalizeUpgrade(component);
- basicUpgradeFinalizer.emitFinishedMsg();
+ finalizer.emitFinishedMsg();
return null;
} catch (Exception e) {
LOG.warn("Upgrade Finalization failed with following Exception:");
Review comment:
Add exception to log message?
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java
##########
@@ -50,37 +52,90 @@
/**
* UpgradeFinalizer implementation for the Storage Container Manager service.
*/
-@SuppressWarnings("checkstyle:VisibilityModifier")
public abstract class BasicUpgradeFinalizer
<T, V extends AbstractLayoutVersionManager> implements UpgradeFinalizer<T> {
- protected V versionManager;
- protected String clientID;
- protected T component;
- protected DefaultUpgradeFinalizationExecutor finalizationExecutor;
+ private V versionManager;
+ private String clientID;
+ private T component;
+ private DefaultUpgradeFinalizationExecutor<T> finalizationExecutor;
private Queue<String> msgs = new ConcurrentLinkedQueue<>();
- protected boolean isDone = false;
+ private boolean isDone = false;
public BasicUpgradeFinalizer(V versionManager) {
this.versionManager = versionManager;
- this.finalizationExecutor =
- new DefaultUpgradeFinalizationExecutor();
+ this.finalizationExecutor = new DefaultUpgradeFinalizationExecutor<>();
}
- /**
- * Sets the Finalization Executor driver.
- * @param executor FinalizationExecutor.
- */
+ public StatusAndMessages finalize(String upgradeClientID, T service)
+ throws IOException {
+ StatusAndMessages response = initFinalize(upgradeClientID, service);
+ if (response.status() != FINALIZATION_REQUIRED) {
+ return response;
+ }
+ finalizationExecutor.execute(service, this);
+ return STARTING_MSG;
+ }
- public void setFinalizationExecutor(DefaultUpgradeFinalizationExecutor
- executor) {
- finalizationExecutor = executor;
+ public synchronized StatusAndMessages reportStatus(
+ String upgradeClientID, boolean takeover) throws UpgradeException {
+ if (takeover) {
+ clientID = upgradeClientID;
+ }
+ assertClientId(upgradeClientID);
+ List<String> returningMsgs = new ArrayList<>(msgs.size() + 10);
+ Status status = versionManager.getUpgradeState();
+ while (msgs.size() > 0) {
+ returningMsgs.add(msgs.poll());
+ }
+ return new StatusAndMessages(status, returningMsgs);
+ }
+
+ protected void preFinalizeUpgrade(T service) throws IOException {
+ // No Op by default.
+ };
+
+ protected void postFinalizeUpgrade(T service) throws IOException {
+ // No Op by default.
}
+ public abstract void finalizeUpgrade(T service) throws UpgradeException;
+
@Override
- public DefaultUpgradeFinalizationExecutor getFinalizationExecutor() {
- return finalizationExecutor;
+ public void finalizeAndWaitForCompletion(
Review comment:
Looks like this only gets called in OzoneManager#instantiateServices, which means that is the only time updateLayoutVersionInDB will be called. Should updateLayoutVersionInDB be called by the finalization executor as part of every successful upgrade? It looks like a normal upgrade of 2/3 OMs will not have a DB update, so when the third gets a snapshot, the version info will not be there.
##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/upgrade/TestHDDSLayoutVersionManager.java
##########
@@ -89,4 +89,14 @@ public void testUpgradeActionsRegistered() throws Exception {
verify(mockObj, times(0)).mockMethodScm();
verify(mockObj, times(1)).mockMethodDn();
}
+
+ @Test
+ public void testHDDSLayoutFeaturesHaveIncreasingLayoutVersion() {
+ HDDSLayoutFeature[] values = HDDSLayoutFeature.values();
+ int currVersion = Integer.MIN_VALUE;
+ for (HDDSLayoutFeature lf : values) {
+ assertTrue(currVersion < lf.layoutVersion());
Review comment:
Do we want to check if the version numbers are consecutive?
##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/upgrade/InjectedUpgradeFinalizationExecutor.java
##########
@@ -66,38 +65,35 @@ public int getValue() {
}
@Override
- public Void execute(Storage storageConfig,
- BasicUpgradeFinalizer basicUpgradeFinalizer)
- throws Exception {
+ public Void execute(T component, BasicUpgradeFinalizer finalizer)
+ throws IOException {
try {
injectTestFunctionAtThisPoint(BEFORE_PRE_FINALIZE_UPGRADE);
- basicUpgradeFinalizer.emitStartingMsg();
- basicUpgradeFinalizer.getVersionManager()
+ finalizer.emitStartingMsg();
+ finalizer.getVersionManager()
.setUpgradeState(FINALIZATION_IN_PROGRESS);
- if(!basicUpgradeFinalizer.preFinalizeUpgrade()) {
- return null;
- }
+ finalizer.preFinalizeUpgrade(component);
injectTestFunctionAtThisPoint(AFTER_PRE_FINALIZE_UPGRADE);
- basicUpgradeFinalizer.finalizeUpgrade(storageConfig);
-
+ finalizer.finalizeUpgrade(component);
injectTestFunctionAtThisPoint(AFTER_COMPLETE_FINALIZATION);
- basicUpgradeFinalizer.postFinalizeUpgrade();
+ finalizer.postFinalizeUpgrade(component);
injectTestFunctionAtThisPoint(AFTER_POST_FINALIZE_UPGRADE);
- basicUpgradeFinalizer.emitFinishedMsg();
+ finalizer.emitFinishedMsg();
+ finalizer.markFinalizationDone();
return null;
} catch (Exception e) {
LOG.warn("Upgrade Finalization failed with following Exception:");
e.printStackTrace();
Review comment:
Add exception to log message?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org