kerneltime commented on code in PR #3534:
URL: https://github.com/apache/ozone/pull/3534#discussion_r906418391
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java:
##########
@@ -46,6 +47,7 @@
import org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes;
import com.google.common.annotations.VisibleForTesting;
+import org.apache.ratis.protocol.exceptions.NotLeaderException;
/**
* UpgradeFinalizer implementation for the Storage Container Manager service.
Review Comment:
Comment needs updating.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java:
##########
@@ -85,14 +87,20 @@ public StatusAndMessages finalize(String upgradeClientID, T
service)
StatusAndMessages response = initFinalize(upgradeClientID, service);
if (finalizationLock.tryLock()) {
try {
- // Even if the status indicates finalization completed, the component
- // may not have finished all its specific steps if finalization was
- // interrupted, so we should re-invoke them here.
+ // If we were able to enter the lock and finalization status is "in
+ // progress", we should resume finalization because the last attempt
+ // was interrupted. If an attempt was currently ongoing, the lock
+ // would have been held.
if (response.status() == FINALIZATION_REQUIRED ||
- !componentFinishedFinalizationSteps(service)) {
+ response.status() == FINALIZATION_IN_PROGRESS) {
finalizationExecutor.execute(service, this);
response = STARTING_MSG;
}
+ // Else, the initial response we got from initFinalize was still
+ // correct.
+ } catch (NotLeaderException e) {
+ LOG.info("Leader change encountered during finalization. This " +
+ "component will continue to finalize as a follower.", e);
Review Comment:
What does this mean? Will the follower continue to finalize as a follower
when the leader executes it, or will the follower continue to finalize on its
own?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/upgrade/DataNodeUpgradeFinalizer.java:
##########
@@ -54,6 +55,8 @@ public void preFinalizeUpgrade(DatanodeStateMachine dsm)
String msg = "Pre Finalization checks failed on the DataNode.";
logAndEmit(msg);
throw new UpgradeException(msg, PREFINALIZE_VALIDATION_FAILED);
+ } else {
Review Comment:
Nit: No need for `else`
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAInvocationHandler.java:
##########
@@ -60,12 +60,19 @@ public SCMHAInvocationHandler(final RequestType requestType,
@Override
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
+ // Javadoc for InvocationHandler#invoke specifies that args will be null
+ // if the method takes no arguments. Convert this to an empty array for
+ // easier handling.
+ Object[] convertedArgs = args;
+ if (args == null) {
+ convertedArgs = new Object[]{};
+ }
Review Comment:
Minor nit:
```suggestion
Object[] convertedArgs = (args == null) ? new Object[]{} : args;
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java:
##########
@@ -46,6 +47,7 @@
import org.apache.hadoop.ozone.upgrade.UpgradeException.ResultCodes;
import com.google.common.annotations.VisibleForTesting;
+import org.apache.ratis.protocol.exceptions.NotLeaderException;
/**
* UpgradeFinalizer implementation for the Storage Container Manager service.
Review Comment:
Nit: Comment needs updating.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java:
##########
@@ -85,14 +87,20 @@ public StatusAndMessages finalize(String upgradeClientID, T
service)
StatusAndMessages response = initFinalize(upgradeClientID, service);
Review Comment:
Should this be inside the lock? 2 threads from 2 different clients trying to
finalize as well as the status for the finalization should be evaluated after
the lock was won.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
