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 2020/11/23 19:09:29 UTC
[GitHub] [ozone] prashantpogde opened a new pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
prashantpogde opened a new pull request #1611:
URL: https://github.com/apache/ozone/pull/1611
## What changes were proposed in this pull request?
Implement post-finalize SCM logic to allow nodes of only new version to participate in pipelines.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4179
## How was this patch tested?
build UT. I will fix failures in CI.
----------------------------------------------------------------
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
[GitHub] [ozone] avijayanhwx commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r529944182
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/exceptions/SCMException.java
##########
@@ -124,6 +124,7 @@ public ResultCodes getResult() {
FAILED_TO_ALLOCATE_ENOUGH_BLOCKS,
INTERNAL_ERROR,
FAILED_TO_INIT_PIPELINE_CHOOSE_POLICY,
- FAILED_TO_INIT_LEADER_CHOOSE_POLICY
+ FAILED_TO_INIT_LEADER_CHOOSE_POLICY,
+ FAILED_TO_FINALIZE_UPGRADE
Review comment:
I don't see any usages for this.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/SCMPipelineManager.java
##########
@@ -268,6 +275,12 @@ public Pipeline createPipeline(ReplicationType type,
}
lock.writeLock().lock();
try {
+ if (freezePipelineCreation.get()) {
+ LOG.debug("Pipeline creation is frozen while an upgrade is in " +
Review comment:
Do we want to throw an exception here? The background pipeline creator will keep trying to create pipelines, and the logs may be filled with exceptions. Is there a way to do this more cleaner? Maybe something like
_Stop pipeline creator --> Add flag in pipeline manager to stop createPipeline (for manual pipeline creation) -> Destroy existing pipelines_
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -1166,4 +1172,107 @@ public String getClusterId() {
public HDDSLayoutVersionManager getLayoutVersionManager() {
return scmLayoutVersionManager;
}
+
+ private void waitForAllContainersToClose() {
Review comment:
This may become a costly operation during finalize. If we have destroyed all pipelines, the containers are automatically closed. So, we maybe able to skip this.
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java
##########
@@ -0,0 +1,271 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.upgrade;
+
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.INVALID_REQUEST;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.LAYOUT_FEATURE_FINALIZATION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.PERSIST_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.REMOVE_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.UPDATE_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_DONE;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_IN_PROGRESS;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+
+import org.apache.hadoop.ozone.common.Storage;
+import org.apache.hadoop.ozone.upgrade.LayoutFeature.UpgradeAction;
+import org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes;
+
+/**
+ * UpgradeFinalizer implementation for the Storage Container Manager service.
+ */
+@SuppressWarnings("checkstyle:VisibilityModifier")
+public class BasicUpgradeFinalizer<T, V extends AbstractLayoutVersionManager>
+ implements UpgradeFinalizer<T> {
+
+ protected V versionManager;
+ protected String clientID;
+ protected T component;
+
+ private Queue<String> msgs = new ConcurrentLinkedQueue<>();
+ protected boolean isDone = false;
+
+ public BasicUpgradeFinalizer(V versionManager) {
+ this.versionManager = versionManager;
+ }
+
+ /*
+ * This method must be overriden by the component implementing the
+ * finalization logic.
+ */
+ public StatusAndMessages finalize(String upgradeClientID, T id)
Review comment:
What happens it this is invoked while the SCM (or OM) is being finalized? If it restarts, we may be OK with losing state, but how about the case when SCM is running & currently finalizing, and that time someone invokes finalize again?
----------------------------------------------------------------
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
[GitHub] [ozone] linyiqun commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
linyiqun commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r529190587
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -1166,4 +1172,107 @@ public String getClusterId() {
public HDDSLayoutVersionManager getLayoutVersionManager() {
return scmLayoutVersionManager;
}
+
+ private void waitForAllContainersToClose() {
+ boolean containersFound = true;
+ while (containersFound) {
+ containersFound = false;
+ for (DatanodeDetails datanodeDetails : scmNodeManager.getAllNodes()) {
+ try {
+ for (ContainerID id : scmNodeManager.getContainers(datanodeDetails)) {
+ try {
+ final ContainerInfo container = containerManager.getContainer(id);
+ if (container.getState() == HddsProtos.LifeCycleState.OPEN ||
+ container.getState() == HddsProtos.LifeCycleState.CLOSING) {
+ containersFound = true;
+ if (container.getState() == HddsProtos.LifeCycleState.OPEN) {
+ eventQueue.fireEvent(SCMEvents.CLOSE_CONTAINER, id);
+ }
+ }
+ } catch (ContainerNotFoundException cnfe) {
+ LOG.warn("Container {} is not managed by ContainerManager.",
+ id, cnfe);
+ continue;
+ }
+ }
+ } catch (NodeNotFoundException e) {
+ continue;
+ }
+ }
+ try {
+ if (containersFound) {
+ LOG.info("Waiting for all containers to close.");
+ Thread.sleep(5000);
+ }
+ } catch (InterruptedException e) {
+ continue;
+ }
+ }
+ }
+
+ private void waitForAllPipelinesToDestroy() throws IOException {
+ boolean pipelineFound = true;
+ while (pipelineFound) {
+ pipelineFound = false;
+ for (Pipeline pipeline : pipelineManager.getPipelines()) {
+ if (pipeline.getPipelineState() != CLOSED) {
+ pipelineFound = true;
+ pipelineManager.finalizeAndDestroyPipeline(pipeline, false);
+ }
+ }
+ try {
+ if (pipelineFound) {
+ LOG.info("Waiting for all pipelines to close.");
+ Thread.sleep(5000);
+ }
+ } catch (InterruptedException e) {
+ continue;
+ }
+ }
+ }
+
+ // This should be called in the context of a separate finalize upgrade thread.
+ // This function can block indefinitely till the conditions are met to safely
+ // finalize Upgrade.
+
+ public void preFinalizeUpgrade() throws IOException {
+ /**
+ * Ask pipeline manager to not create any new pipelines. Pipeline
+ * creation will remain frozen until postFinalizeUpgrade().
+ */
+ pipelineManager.freezePipelineCreation();
+
Review comment:
Can we also reset SCMSafeModeManager#setInSafeMode to true? In some scenarios, we alreadt use safemode status check to reject some create type operations for new containers/pipelines.
----------------------------------------------------------------
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
[GitHub] [ozone] linyiqun commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
linyiqun commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530280726
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -1166,4 +1172,107 @@ public String getClusterId() {
public HDDSLayoutVersionManager getLayoutVersionManager() {
return scmLayoutVersionManager;
}
+
+ private void waitForAllContainersToClose() {
+ boolean containersFound = true;
+ while (containersFound) {
+ containersFound = false;
+ for (DatanodeDetails datanodeDetails : scmNodeManager.getAllNodes()) {
+ try {
+ for (ContainerID id : scmNodeManager.getContainers(datanodeDetails)) {
+ try {
+ final ContainerInfo container = containerManager.getContainer(id);
+ if (container.getState() == HddsProtos.LifeCycleState.OPEN ||
+ container.getState() == HddsProtos.LifeCycleState.CLOSING) {
+ containersFound = true;
+ if (container.getState() == HddsProtos.LifeCycleState.OPEN) {
+ eventQueue.fireEvent(SCMEvents.CLOSE_CONTAINER, id);
+ }
+ }
+ } catch (ContainerNotFoundException cnfe) {
+ LOG.warn("Container {} is not managed by ContainerManager.",
+ id, cnfe);
+ continue;
+ }
+ }
+ } catch (NodeNotFoundException e) {
+ continue;
+ }
+ }
+ try {
+ if (containersFound) {
+ LOG.info("Waiting for all containers to close.");
+ Thread.sleep(5000);
+ }
+ } catch (InterruptedException e) {
+ continue;
+ }
+ }
+ }
+
+ private void waitForAllPipelinesToDestroy() throws IOException {
+ boolean pipelineFound = true;
+ while (pipelineFound) {
+ pipelineFound = false;
+ for (Pipeline pipeline : pipelineManager.getPipelines()) {
+ if (pipeline.getPipelineState() != CLOSED) {
+ pipelineFound = true;
+ pipelineManager.finalizeAndDestroyPipeline(pipeline, false);
+ }
+ }
+ try {
+ if (pipelineFound) {
+ LOG.info("Waiting for all pipelines to close.");
+ Thread.sleep(5000);
+ }
+ } catch (InterruptedException e) {
+ continue;
+ }
+ }
+ }
+
+ // This should be called in the context of a separate finalize upgrade thread.
+ // This function can block indefinitely till the conditions are met to safely
+ // finalize Upgrade.
+
+ public void preFinalizeUpgrade() throws IOException {
+ /**
+ * Ask pipeline manager to not create any new pipelines. Pipeline
+ * creation will remain frozen until postFinalizeUpgrade().
+ */
+ pipelineManager.freezePipelineCreation();
+
Review comment:
Okay, makes sense 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.
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
[GitHub] [ozone] fapifta commented on pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#issuecomment-738108520
Thanks @prashantpogde for working on this one, and addressing the review comments.
The changes look good to me, as we discussed the proposed refactoring is something we might get back to but we would defer it for now.
+1 from my side.
----------------------------------------------------------------
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
[GitHub] [ozone] fapifta commented on pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#issuecomment-734014080
Even though the wrapper for OM does nothing in the pre/post hook, it might worth to write the code once and just give an empty implementation for the hooks. Sure let's discuss this one during one of our next calls.
----------------------------------------------------------------
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
[GitHub] [ozone] prashantpogde commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530107012
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -1166,4 +1172,107 @@ public String getClusterId() {
public HDDSLayoutVersionManager getLayoutVersionManager() {
return scmLayoutVersionManager;
}
+
+ private void waitForAllContainersToClose() {
+ boolean containersFound = true;
+ while (containersFound) {
+ containersFound = false;
+ for (DatanodeDetails datanodeDetails : scmNodeManager.getAllNodes()) {
+ try {
+ for (ContainerID id : scmNodeManager.getContainers(datanodeDetails)) {
+ try {
+ final ContainerInfo container = containerManager.getContainer(id);
+ if (container.getState() == HddsProtos.LifeCycleState.OPEN ||
+ container.getState() == HddsProtos.LifeCycleState.CLOSING) {
+ containersFound = true;
+ if (container.getState() == HddsProtos.LifeCycleState.OPEN) {
+ eventQueue.fireEvent(SCMEvents.CLOSE_CONTAINER, id);
+ }
+ }
+ } catch (ContainerNotFoundException cnfe) {
+ LOG.warn("Container {} is not managed by ContainerManager.",
+ id, cnfe);
+ continue;
+ }
+ }
+ } catch (NodeNotFoundException e) {
+ continue;
+ }
+ }
+ try {
+ if (containersFound) {
+ LOG.info("Waiting for all containers to close.");
+ Thread.sleep(5000);
+ }
+ } catch (InterruptedException e) {
+ continue;
+ }
+ }
+ }
+
+ private void waitForAllPipelinesToDestroy() throws IOException {
+ boolean pipelineFound = true;
+ while (pipelineFound) {
+ pipelineFound = false;
+ for (Pipeline pipeline : pipelineManager.getPipelines()) {
+ if (pipeline.getPipelineState() != CLOSED) {
+ pipelineFound = true;
+ pipelineManager.finalizeAndDestroyPipeline(pipeline, false);
+ }
+ }
+ try {
+ if (pipelineFound) {
+ LOG.info("Waiting for all pipelines to close.");
+ Thread.sleep(5000);
+ }
+ } catch (InterruptedException e) {
+ continue;
+ }
+ }
+ }
+
+ // This should be called in the context of a separate finalize upgrade thread.
+ // This function can block indefinitely till the conditions are met to safely
+ // finalize Upgrade.
+
+ public void preFinalizeUpgrade() throws IOException {
+ /**
+ * Ask pipeline manager to not create any new pipelines. Pipeline
+ * creation will remain frozen until postFinalizeUpgrade().
+ */
+ pipelineManager.freezePipelineCreation();
+
Review comment:
That is another route but we will need stop SCM from coming out of safe mode until upgrade is complete. Also safe mode is good to enter when the SCM is starts. Here the SCM was already running and I just needed a way to pause pipeline creation for an upgrade window.
----------------------------------------------------------------
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
[GitHub] [ozone] fapifta commented on pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#issuecomment-733932797
I am thinking about the refactor of the UpgradeFinalizer.
In the BasicFinalizer, there is a finalizeFeature, that is called from the SCMUpgradeFinalizer, while the OMUpgradeFinalizer has its own finalizeFeature method.
What if we generalize this one to be the one in BasicUpgradeFinalizer along with the worker, or why don't we?
Roughly this is what I would try:
- BasicUpgradeFinalizer defines the Worker, and we encapsulate the component into a wrapper behind an interface and use that in the worker.
- the component's wrapper should have the two hooks, preFinalizeUpgrade and postFinalizeUpgrade
- if we add a getStorage method we can avoid the problem that comes from the different Storage getters as well
- we can add any other required stuff into the wrapper, and cut off the upgrade finalization logic from the component's implementation without cutting the UpgradeActions, as there we can still provide the component from the wrapper.
So at the end of the day we may get to one generic UpgradeFinalizer, that uses a let's say UpgradingComponent interface for which the implementations may be OzoneManagerComponent, and StorageContainerManagerComponent.
I started to think about this because of the similarities in the two concrete UpgradeFinalizer, and the two pretty much different approach on how we handled the Optional UpgradeAction :)
----------------------------------------------------------------
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
[GitHub] [ozone] prashantpogde commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530707897
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java
##########
@@ -0,0 +1,300 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.upgrade;
+
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.INVALID_REQUEST;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.LAYOUT_FEATURE_FINALIZATION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.PERSIST_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.REMOVE_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.UPDATE_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_DONE;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_IN_PROGRESS;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_REQUIRED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.STARTING_FINALIZATION;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+
+import org.apache.hadoop.ozone.common.Storage;
+import org.apache.hadoop.ozone.upgrade.LayoutFeature.UpgradeAction;
+import org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes;
+
+/**
+ * UpgradeFinalizer implementation for the Storage Container Manager service.
+ */
+@SuppressWarnings("checkstyle:VisibilityModifier")
+public class BasicUpgradeFinalizer<T, V extends AbstractLayoutVersionManager>
+ implements UpgradeFinalizer<T> {
+
+ protected V versionManager;
+ protected String clientID;
+ protected T component;
+
+ private Queue<String> msgs = new ConcurrentLinkedQueue<>();
+ protected boolean isDone = false;
+
+ public BasicUpgradeFinalizer(V versionManager) {
+ this.versionManager = versionManager;
+ }
+
+ public StatusAndMessages preFinalize(String upgradeClientID, T id)
+ throws UpgradeException {
+ switch (versionManager.getUpgradeState()) {
+ case STARTING_FINALIZATION:
+ return STARTING_MSG;
+ case FINALIZATION_IN_PROGRESS:
+ return FINALIZATION_IN_PROGRESS_MSG;
+ case FINALIZATION_DONE:
+ case ALREADY_FINALIZED:
+ return FINALIZED_MSG;
+ default:
+ if (!versionManager.needsFinalization()) {
+ throw new UpgradeException("Upgrade found in inconsistent state. " +
+ "Upgrade state is FINALIZATION_REQUIRED while MLV has been " +
+ "upgraded to SLV.", INVALID_REQUEST);
+ }
+ versionManager.setUpgradeState(STARTING_FINALIZATION);
+
+ clientID = upgradeClientID;
+ this.component = id;
+ return FINALIZATION_REQUIRED_MSG;
+ }
+ }
+
+ /*
+ * This method must be overriden by the component implementing the
+ * finalization logic.
+ */
+ public StatusAndMessages finalize(String upgradeClientID, T id)
+ throws IOException {
+ StatusAndMessages response = preFinalize(upgradeClientID, id);
+ if (response.status() != FINALIZATION_REQUIRED) {
+ return response;
+ }
+
+ /**
+ * Overriding class should schedule actual finalization logic
+ * in a separate thread here.
+ */
+ return STARTING_MSG;
+ }
+
+ @Override
+ 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 = isDone ? FINALIZATION_DONE : FINALIZATION_IN_PROGRESS;
+ while (msgs.size() > 0) {
+ returningMsgs.add(msgs.poll());
+ }
+ return new StatusAndMessages(status, returningMsgs);
+ }
+
+ private void assertClientId(String id) throws UpgradeException {
+ if (!this.clientID.equals(id)) {
+ throw new UpgradeException("Unknown client tries to get finalization " +
+ "status.\n The requestor is not the initiating client of the " +
+ "finalization, if you want to take over, and get unsent status " +
+ "messages, check -takeover option.", INVALID_REQUEST);
+ }
+ }
+
+ protected void finalizeFeature(LayoutFeature feature, Storage config)
+ throws UpgradeException {
+ Optional<? extends UpgradeAction> action = feature.onFinalizeAction();
+
+ if (action.isPresent()) {
Review comment:
why do we want to do noop ? action is not present we do not have to do anything.
----------------------------------------------------------------
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
[GitHub] [ozone] fapifta commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
fapifta commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530605858
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java
##########
@@ -0,0 +1,300 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.upgrade;
+
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.INVALID_REQUEST;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.LAYOUT_FEATURE_FINALIZATION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.PERSIST_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.REMOVE_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.UPDATE_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_DONE;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_IN_PROGRESS;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_REQUIRED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.STARTING_FINALIZATION;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+
+import org.apache.hadoop.ozone.common.Storage;
+import org.apache.hadoop.ozone.upgrade.LayoutFeature.UpgradeAction;
+import org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes;
+
+/**
+ * UpgradeFinalizer implementation for the Storage Container Manager service.
+ */
+@SuppressWarnings("checkstyle:VisibilityModifier")
+public class BasicUpgradeFinalizer<T, V extends AbstractLayoutVersionManager>
+ implements UpgradeFinalizer<T> {
+
+ protected V versionManager;
+ protected String clientID;
+ protected T component;
+
+ private Queue<String> msgs = new ConcurrentLinkedQueue<>();
+ protected boolean isDone = false;
+
+ public BasicUpgradeFinalizer(V versionManager) {
+ this.versionManager = versionManager;
+ }
+
+ public StatusAndMessages preFinalize(String upgradeClientID, T id)
+ throws UpgradeException {
+ switch (versionManager.getUpgradeState()) {
+ case STARTING_FINALIZATION:
+ return STARTING_MSG;
+ case FINALIZATION_IN_PROGRESS:
+ return FINALIZATION_IN_PROGRESS_MSG;
+ case FINALIZATION_DONE:
+ case ALREADY_FINALIZED:
+ return FINALIZED_MSG;
+ default:
+ if (!versionManager.needsFinalization()) {
+ throw new UpgradeException("Upgrade found in inconsistent state. " +
+ "Upgrade state is FINALIZATION_REQUIRED while MLV has been " +
+ "upgraded to SLV.", INVALID_REQUEST);
+ }
+ versionManager.setUpgradeState(STARTING_FINALIZATION);
+
+ clientID = upgradeClientID;
+ this.component = id;
+ return FINALIZATION_REQUIRED_MSG;
+ }
+ }
+
+ /*
+ * This method must be overriden by the component implementing the
+ * finalization logic.
+ */
+ public StatusAndMessages finalize(String upgradeClientID, T id)
+ throws IOException {
+ StatusAndMessages response = preFinalize(upgradeClientID, id);
+ if (response.status() != FINALIZATION_REQUIRED) {
+ return response;
+ }
+
+ /**
+ * Overriding class should schedule actual finalization logic
+ * in a separate thread here.
+ */
+ return STARTING_MSG;
+ }
+
+ @Override
+ 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 = isDone ? FINALIZATION_DONE : FINALIZATION_IN_PROGRESS;
+ while (msgs.size() > 0) {
+ returningMsgs.add(msgs.poll());
+ }
+ return new StatusAndMessages(status, returningMsgs);
+ }
+
+ private void assertClientId(String id) throws UpgradeException {
+ if (!this.clientID.equals(id)) {
+ throw new UpgradeException("Unknown client tries to get finalization " +
+ "status.\n The requestor is not the initiating client of the " +
+ "finalization, if you want to take over, and get unsent status " +
+ "messages, check -takeover option.", INVALID_REQUEST);
+ }
+ }
+
+ protected void finalizeFeature(LayoutFeature feature, Storage config)
+ throws UpgradeException {
+ Optional<? extends UpgradeAction> action = feature.onFinalizeAction();
+
+ if (action.isPresent()) {
Review comment:
Aren't we want to do a noop when the action is not present?
----------------------------------------------------------------
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
[GitHub] [ozone] fapifta commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
fapifta commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530713077
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java
##########
@@ -0,0 +1,300 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.upgrade;
+
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.INVALID_REQUEST;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.LAYOUT_FEATURE_FINALIZATION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.PERSIST_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.REMOVE_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.UPDATE_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_DONE;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_IN_PROGRESS;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_REQUIRED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.STARTING_FINALIZATION;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+
+import org.apache.hadoop.ozone.common.Storage;
+import org.apache.hadoop.ozone.upgrade.LayoutFeature.UpgradeAction;
+import org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes;
+
+/**
+ * UpgradeFinalizer implementation for the Storage Container Manager service.
+ */
+@SuppressWarnings("checkstyle:VisibilityModifier")
+public class BasicUpgradeFinalizer<T, V extends AbstractLayoutVersionManager>
+ implements UpgradeFinalizer<T> {
+
+ protected V versionManager;
+ protected String clientID;
+ protected T component;
+
+ private Queue<String> msgs = new ConcurrentLinkedQueue<>();
+ protected boolean isDone = false;
+
+ public BasicUpgradeFinalizer(V versionManager) {
+ this.versionManager = versionManager;
+ }
+
+ public StatusAndMessages preFinalize(String upgradeClientID, T id)
+ throws UpgradeException {
+ switch (versionManager.getUpgradeState()) {
+ case STARTING_FINALIZATION:
+ return STARTING_MSG;
+ case FINALIZATION_IN_PROGRESS:
+ return FINALIZATION_IN_PROGRESS_MSG;
+ case FINALIZATION_DONE:
+ case ALREADY_FINALIZED:
+ return FINALIZED_MSG;
+ default:
+ if (!versionManager.needsFinalization()) {
+ throw new UpgradeException("Upgrade found in inconsistent state. " +
+ "Upgrade state is FINALIZATION_REQUIRED while MLV has been " +
+ "upgraded to SLV.", INVALID_REQUEST);
+ }
+ versionManager.setUpgradeState(STARTING_FINALIZATION);
+
+ clientID = upgradeClientID;
+ this.component = id;
+ return FINALIZATION_REQUIRED_MSG;
+ }
+ }
+
+ /*
+ * This method must be overriden by the component implementing the
+ * finalization logic.
+ */
+ public StatusAndMessages finalize(String upgradeClientID, T id)
+ throws IOException {
+ StatusAndMessages response = preFinalize(upgradeClientID, id);
+ if (response.status() != FINALIZATION_REQUIRED) {
+ return response;
+ }
+
+ /**
+ * Overriding class should schedule actual finalization logic
+ * in a separate thread here.
+ */
+ return STARTING_MSG;
+ }
+
+ @Override
+ 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 = isDone ? FINALIZATION_DONE : FINALIZATION_IN_PROGRESS;
+ while (msgs.size() > 0) {
+ returningMsgs.add(msgs.poll());
+ }
+ return new StatusAndMessages(status, returningMsgs);
+ }
+
+ private void assertClientId(String id) throws UpgradeException {
+ if (!this.clientID.equals(id)) {
+ throw new UpgradeException("Unknown client tries to get finalization " +
+ "status.\n The requestor is not the initiating client of the " +
+ "finalization, if you want to take over, and get unsent status " +
+ "messages, check -takeover option.", INVALID_REQUEST);
+ }
+ }
+
+ protected void finalizeFeature(LayoutFeature feature, Storage config)
+ throws UpgradeException {
+ Optional<? extends UpgradeAction> action = feature.onFinalizeAction();
+
+ if (action.isPresent()) {
Review comment:
Sorry for this, but my wording was not correct as it seems.
Let me put it this way:
Here if action is present in the optional, we emit a noop message and return, I believe we want to emit a noop message and return if action.isPresent yields is false, so we miss a negation in the condition.
----------------------------------------------------------------
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
[GitHub] [ozone] prashantpogde commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530589868
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java
##########
@@ -0,0 +1,271 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.upgrade;
+
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.INVALID_REQUEST;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.LAYOUT_FEATURE_FINALIZATION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.PERSIST_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.REMOVE_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.UPDATE_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_DONE;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_IN_PROGRESS;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+
+import org.apache.hadoop.ozone.common.Storage;
+import org.apache.hadoop.ozone.upgrade.LayoutFeature.UpgradeAction;
+import org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes;
+
+/**
+ * UpgradeFinalizer implementation for the Storage Container Manager service.
+ */
+@SuppressWarnings("checkstyle:VisibilityModifier")
+public class BasicUpgradeFinalizer<T, V extends AbstractLayoutVersionManager>
+ implements UpgradeFinalizer<T> {
+
+ protected V versionManager;
+ protected String clientID;
+ protected T component;
+
+ private Queue<String> msgs = new ConcurrentLinkedQueue<>();
+ protected boolean isDone = false;
+
+ public BasicUpgradeFinalizer(V versionManager) {
+ this.versionManager = versionManager;
+ }
+
+ /*
+ * This method must be overriden by the component implementing the
+ * finalization logic.
+ */
+ public StatusAndMessages finalize(String upgradeClientID, T id)
Review comment:
yes, there is a. clientId based check. But I will add additional safeguards to prevent this.
----------------------------------------------------------------
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
[GitHub] [ozone] linyiqun commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
linyiqun commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530285817
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -1166,4 +1172,107 @@ public String getClusterId() {
public HDDSLayoutVersionManager getLayoutVersionManager() {
return scmLayoutVersionManager;
}
+
+ private void waitForAllContainersToClose() {
Review comment:
Agree the point that @avijayanhwx mentioned. With the number of containers/pipeline becomes more large, this operation cal be very costly. can we just add a flag switch to avoid write type ops for container/pipelines, like freezePipelineCreation we added above.
----------------------------------------------------------------
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
[GitHub] [ozone] prashantpogde commented on pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#issuecomment-734008365
>
> I am thinking about the refactor of the UpgradeFinalizer.
> In the BasicFinalizer, there is a finalizeFeature, that is called from the SCMUpgradeFinalizer, while the OMUpgradeFinalizer has its own finalizeFeature method.
....
....
> I started to think about this because of the similarities in the two concrete UpgradeFinalizer, and the two pretty much different approach on how we handled the Optional UpgradeAction :
Earlier I was keeping worker also in the Basic UpgradeFinalizer as you suggested but it seemed Om finalization didnt need pre/post hook. So left it this way. We can also discuss this in a call.
----------------------------------------------------------------
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
[GitHub] [ozone] avijayanhwx commented on pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#issuecomment-738174577
CI failure is on OM side, while the changes are on HDDS side. Unable to repro this on my local setup.
----------------------------------------------------------------
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
[GitHub] [ozone] prashantpogde commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530109591
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -1166,4 +1172,107 @@ public String getClusterId() {
public HDDSLayoutVersionManager getLayoutVersionManager() {
return scmLayoutVersionManager;
}
+
+ private void waitForAllContainersToClose() {
Review comment:
Yes, but shouldnt we wait for all containers to close before we finalize the upgrade ? Because as part of closing the containers wemay excercise older layout version of the code otherwirse.
----------------------------------------------------------------
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
[GitHub] [ozone] fapifta commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
fapifta commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530713077
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java
##########
@@ -0,0 +1,300 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.upgrade;
+
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.INVALID_REQUEST;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.LAYOUT_FEATURE_FINALIZATION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.PERSIST_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.REMOVE_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.UPDATE_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_DONE;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_IN_PROGRESS;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_REQUIRED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.STARTING_FINALIZATION;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+
+import org.apache.hadoop.ozone.common.Storage;
+import org.apache.hadoop.ozone.upgrade.LayoutFeature.UpgradeAction;
+import org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes;
+
+/**
+ * UpgradeFinalizer implementation for the Storage Container Manager service.
+ */
+@SuppressWarnings("checkstyle:VisibilityModifier")
+public class BasicUpgradeFinalizer<T, V extends AbstractLayoutVersionManager>
+ implements UpgradeFinalizer<T> {
+
+ protected V versionManager;
+ protected String clientID;
+ protected T component;
+
+ private Queue<String> msgs = new ConcurrentLinkedQueue<>();
+ protected boolean isDone = false;
+
+ public BasicUpgradeFinalizer(V versionManager) {
+ this.versionManager = versionManager;
+ }
+
+ public StatusAndMessages preFinalize(String upgradeClientID, T id)
+ throws UpgradeException {
+ switch (versionManager.getUpgradeState()) {
+ case STARTING_FINALIZATION:
+ return STARTING_MSG;
+ case FINALIZATION_IN_PROGRESS:
+ return FINALIZATION_IN_PROGRESS_MSG;
+ case FINALIZATION_DONE:
+ case ALREADY_FINALIZED:
+ return FINALIZED_MSG;
+ default:
+ if (!versionManager.needsFinalization()) {
+ throw new UpgradeException("Upgrade found in inconsistent state. " +
+ "Upgrade state is FINALIZATION_REQUIRED while MLV has been " +
+ "upgraded to SLV.", INVALID_REQUEST);
+ }
+ versionManager.setUpgradeState(STARTING_FINALIZATION);
+
+ clientID = upgradeClientID;
+ this.component = id;
+ return FINALIZATION_REQUIRED_MSG;
+ }
+ }
+
+ /*
+ * This method must be overriden by the component implementing the
+ * finalization logic.
+ */
+ public StatusAndMessages finalize(String upgradeClientID, T id)
+ throws IOException {
+ StatusAndMessages response = preFinalize(upgradeClientID, id);
+ if (response.status() != FINALIZATION_REQUIRED) {
+ return response;
+ }
+
+ /**
+ * Overriding class should schedule actual finalization logic
+ * in a separate thread here.
+ */
+ return STARTING_MSG;
+ }
+
+ @Override
+ 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 = isDone ? FINALIZATION_DONE : FINALIZATION_IN_PROGRESS;
+ while (msgs.size() > 0) {
+ returningMsgs.add(msgs.poll());
+ }
+ return new StatusAndMessages(status, returningMsgs);
+ }
+
+ private void assertClientId(String id) throws UpgradeException {
+ if (!this.clientID.equals(id)) {
+ throw new UpgradeException("Unknown client tries to get finalization " +
+ "status.\n The requestor is not the initiating client of the " +
+ "finalization, if you want to take over, and get unsent status " +
+ "messages, check -takeover option.", INVALID_REQUEST);
+ }
+ }
+
+ protected void finalizeFeature(LayoutFeature feature, Storage config)
+ throws UpgradeException {
+ Optional<? extends UpgradeAction> action = feature.onFinalizeAction();
+
+ if (action.isPresent()) {
Review comment:
Sorry for this, but my wording was not correct as it seems.
Let me put it this way:
Here if action is present in the optional, we emit a noop message and return, I believe we want to emit a noop message and return if action.isPresent is false, so we miss a negation in the condition.
----------------------------------------------------------------
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
[GitHub] [ozone] prashantpogde commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530107157
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/exceptions/SCMException.java
##########
@@ -124,6 +124,7 @@ public ResultCodes getResult() {
FAILED_TO_ALLOCATE_ENOUGH_BLOCKS,
INTERNAL_ERROR,
FAILED_TO_INIT_PIPELINE_CHOOSE_POLICY,
- FAILED_TO_INIT_LEADER_CHOOSE_POLICY
+ FAILED_TO_INIT_LEADER_CHOOSE_POLICY,
+ FAILED_TO_FINALIZE_UPGRADE
Review comment:
yes, left over code when I added UpgradeException. I will remove this.
----------------------------------------------------------------
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
[GitHub] [ozone] prashantpogde commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r532966581
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java
##########
@@ -0,0 +1,300 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.upgrade;
+
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.INVALID_REQUEST;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.LAYOUT_FEATURE_FINALIZATION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.PERSIST_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.REMOVE_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.UPDATE_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_DONE;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_IN_PROGRESS;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_REQUIRED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.STARTING_FINALIZATION;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+
+import org.apache.hadoop.ozone.common.Storage;
+import org.apache.hadoop.ozone.upgrade.LayoutFeature.UpgradeAction;
+import org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes;
+
+/**
+ * UpgradeFinalizer implementation for the Storage Container Manager service.
+ */
+@SuppressWarnings("checkstyle:VisibilityModifier")
+public class BasicUpgradeFinalizer<T, V extends AbstractLayoutVersionManager>
+ implements UpgradeFinalizer<T> {
+
+ protected V versionManager;
+ protected String clientID;
+ protected T component;
+
+ private Queue<String> msgs = new ConcurrentLinkedQueue<>();
+ protected boolean isDone = false;
+
+ public BasicUpgradeFinalizer(V versionManager) {
+ this.versionManager = versionManager;
+ }
+
+ public StatusAndMessages preFinalize(String upgradeClientID, T id)
+ throws UpgradeException {
+ switch (versionManager.getUpgradeState()) {
+ case STARTING_FINALIZATION:
+ return STARTING_MSG;
+ case FINALIZATION_IN_PROGRESS:
+ return FINALIZATION_IN_PROGRESS_MSG;
+ case FINALIZATION_DONE:
+ case ALREADY_FINALIZED:
+ return FINALIZED_MSG;
+ default:
+ if (!versionManager.needsFinalization()) {
+ throw new UpgradeException("Upgrade found in inconsistent state. " +
+ "Upgrade state is FINALIZATION_REQUIRED while MLV has been " +
+ "upgraded to SLV.", INVALID_REQUEST);
+ }
+ versionManager.setUpgradeState(STARTING_FINALIZATION);
+
+ clientID = upgradeClientID;
+ this.component = id;
+ return FINALIZATION_REQUIRED_MSG;
+ }
+ }
+
+ /*
+ * This method must be overriden by the component implementing the
+ * finalization logic.
+ */
+ public StatusAndMessages finalize(String upgradeClientID, T id)
+ throws IOException {
+ StatusAndMessages response = preFinalize(upgradeClientID, id);
+ if (response.status() != FINALIZATION_REQUIRED) {
+ return response;
+ }
+
+ /**
+ * Overriding class should schedule actual finalization logic
+ * in a separate thread here.
+ */
+ return STARTING_MSG;
+ }
+
+ @Override
+ 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 = isDone ? FINALIZATION_DONE : FINALIZATION_IN_PROGRESS;
+ while (msgs.size() > 0) {
+ returningMsgs.add(msgs.poll());
+ }
+ return new StatusAndMessages(status, returningMsgs);
+ }
+
+ private void assertClientId(String id) throws UpgradeException {
+ if (!this.clientID.equals(id)) {
+ throw new UpgradeException("Unknown client tries to get finalization " +
+ "status.\n The requestor is not the initiating client of the " +
+ "finalization, if you want to take over, and get unsent status " +
+ "messages, check -takeover option.", INVALID_REQUEST);
+ }
+ }
+
+ protected void finalizeFeature(LayoutFeature feature, Storage config)
+ throws UpgradeException {
+ Optional<? extends UpgradeAction> action = feature.onFinalizeAction();
+
+ if (action.isPresent()) {
Review comment:
yes, I see now :) , I will fix it.
----------------------------------------------------------------
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
[GitHub] [ozone] prashantpogde commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530707450
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStateManager.java
##########
@@ -289,7 +292,7 @@ private void initialiseState2EventMap() {
*/
private void initializeStateMachine() {
stateMachine.addTransition(
- NodeState.HEALTHY_READONLY, NodeState.HEALTHY,
+ NodeState.HEALTHY_READONLY, HEALTHY,
NodeLifeCycleEvent.LAYOUT_MATCH);
Review comment:
yup, will 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.
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
[GitHub] [ozone] prashantpogde commented on pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#issuecomment-736997665
TestOmBlockVersioning failure is unrelated. Its passing on my local setup.
----------------------------------------------------------------
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
[GitHub] [ozone] fapifta commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
fapifta commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530601743
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStateManager.java
##########
@@ -289,7 +292,7 @@ private void initialiseState2EventMap() {
*/
private void initializeStateMachine() {
stateMachine.addTransition(
- NodeState.HEALTHY_READONLY, NodeState.HEALTHY,
+ NodeState.HEALTHY_READONLY, HEALTHY,
NodeLifeCycleEvent.LAYOUT_MATCH);
Review comment:
Nit: as we statically import HEALTHY_READONLY, we can leave out the qualification of NodeState here, as we do in forceNodesToHealthyReadOnly
----------------------------------------------------------------
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
[GitHub] [ozone] prashantpogde commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r532966581
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java
##########
@@ -0,0 +1,300 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.upgrade;
+
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.INVALID_REQUEST;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.LAYOUT_FEATURE_FINALIZATION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.PERSIST_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.REMOVE_UPGRADE_TO_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes.UPDATE_LAYOUT_VERSION_FAILED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_DONE;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_IN_PROGRESS;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.FINALIZATION_REQUIRED;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalizer.Status.STARTING_FINALIZATION;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+
+import org.apache.hadoop.ozone.common.Storage;
+import org.apache.hadoop.ozone.upgrade.LayoutFeature.UpgradeAction;
+import org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes;
+
+/**
+ * UpgradeFinalizer implementation for the Storage Container Manager service.
+ */
+@SuppressWarnings("checkstyle:VisibilityModifier")
+public class BasicUpgradeFinalizer<T, V extends AbstractLayoutVersionManager>
+ implements UpgradeFinalizer<T> {
+
+ protected V versionManager;
+ protected String clientID;
+ protected T component;
+
+ private Queue<String> msgs = new ConcurrentLinkedQueue<>();
+ protected boolean isDone = false;
+
+ public BasicUpgradeFinalizer(V versionManager) {
+ this.versionManager = versionManager;
+ }
+
+ public StatusAndMessages preFinalize(String upgradeClientID, T id)
+ throws UpgradeException {
+ switch (versionManager.getUpgradeState()) {
+ case STARTING_FINALIZATION:
+ return STARTING_MSG;
+ case FINALIZATION_IN_PROGRESS:
+ return FINALIZATION_IN_PROGRESS_MSG;
+ case FINALIZATION_DONE:
+ case ALREADY_FINALIZED:
+ return FINALIZED_MSG;
+ default:
+ if (!versionManager.needsFinalization()) {
+ throw new UpgradeException("Upgrade found in inconsistent state. " +
+ "Upgrade state is FINALIZATION_REQUIRED while MLV has been " +
+ "upgraded to SLV.", INVALID_REQUEST);
+ }
+ versionManager.setUpgradeState(STARTING_FINALIZATION);
+
+ clientID = upgradeClientID;
+ this.component = id;
+ return FINALIZATION_REQUIRED_MSG;
+ }
+ }
+
+ /*
+ * This method must be overriden by the component implementing the
+ * finalization logic.
+ */
+ public StatusAndMessages finalize(String upgradeClientID, T id)
+ throws IOException {
+ StatusAndMessages response = preFinalize(upgradeClientID, id);
+ if (response.status() != FINALIZATION_REQUIRED) {
+ return response;
+ }
+
+ /**
+ * Overriding class should schedule actual finalization logic
+ * in a separate thread here.
+ */
+ return STARTING_MSG;
+ }
+
+ @Override
+ 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 = isDone ? FINALIZATION_DONE : FINALIZATION_IN_PROGRESS;
+ while (msgs.size() > 0) {
+ returningMsgs.add(msgs.poll());
+ }
+ return new StatusAndMessages(status, returningMsgs);
+ }
+
+ private void assertClientId(String id) throws UpgradeException {
+ if (!this.clientID.equals(id)) {
+ throw new UpgradeException("Unknown client tries to get finalization " +
+ "status.\n The requestor is not the initiating client of the " +
+ "finalization, if you want to take over, and get unsent status " +
+ "messages, check -takeover option.", INVALID_REQUEST);
+ }
+ }
+
+ protected void finalizeFeature(LayoutFeature feature, Storage config)
+ throws UpgradeException {
+ Optional<? extends UpgradeAction> action = feature.onFinalizeAction();
+
+ if (action.isPresent()) {
Review comment:
yes, I see now :)
----------------------------------------------------------------
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
[GitHub] [ozone] fapifta edited a comment on pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
fapifta edited a comment on pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#issuecomment-733932797
Hi Prashant, thank you for working on this one, great work so far!
I am thinking about the refactor of the UpgradeFinalizer.
In the BasicFinalizer, there is a finalizeFeature, that is called from the SCMUpgradeFinalizer, while the OMUpgradeFinalizer has its own finalizeFeature method.
What if we generalize this one to be the one in BasicUpgradeFinalizer along with the worker, or why don't we?
Roughly this is what I would try:
- BasicUpgradeFinalizer defines the Worker, and we encapsulate the component into a wrapper behind an interface and use that in the worker.
- the component's wrapper should have the two hooks, preFinalizeUpgrade and postFinalizeUpgrade
- if we add a getStorage method we can avoid the problem that comes from the different Storage getters as well
- we can add any other required stuff into the wrapper, and cut off the upgrade finalization logic from the component's implementation without cutting the UpgradeActions, as there we can still provide the component from the wrapper.
So at the end of the day we may get to one generic UpgradeFinalizer, that uses a let's say UpgradingComponent interface for which the implementations may be OzoneManagerComponent, and StorageContainerManagerComponent.
I started to think about this because of the similarities in the two concrete UpgradeFinalizer, and the two pretty much different approach on how we handled the Optional UpgradeAction :)
----------------------------------------------------------------
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
[GitHub] [ozone] prashantpogde commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r532964803
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -1166,4 +1172,107 @@ public String getClusterId() {
public HDDSLayoutVersionManager getLayoutVersionManager() {
return scmLayoutVersionManager;
}
+
+ private void waitForAllContainersToClose() {
Review comment:
Agreed and removed the "waitForAllContainersToClose()" call. Leaving the function around for now just in case we need it in future.
----------------------------------------------------------------
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
[GitHub] [ozone] prashantpogde commented on a change in pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on a change in pull request #1611:
URL: https://github.com/apache/ozone/pull/1611#discussion_r530109093
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/SCMPipelineManager.java
##########
@@ -268,6 +275,12 @@ public Pipeline createPipeline(ReplicationType type,
}
lock.writeLock().lock();
try {
+ if (freezePipelineCreation.get()) {
+ LOG.debug("Pipeline creation is frozen while an upgrade is in " +
Review comment:
OZONE_SCM_PIPELINE_CREATION_INTERVAL_DEFAULT is 2 minutes. I thought that upgrade finalization should be few minutes. Its possible that we may have few messages in the log to indicate that pipeline creation is frozen due to upgrade. But I can add a "pause" for pipeline creator as suggested.
----------------------------------------------------------------
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
[GitHub] [ozone] avijayanhwx merged pull request #1611: HDDS-4179. Implement post-finalize SCM logic.
Posted by GitBox <gi...@apache.org>.
avijayanhwx merged pull request #1611:
URL: https://github.com/apache/ozone/pull/1611
----------------------------------------------------------------
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