errose28 commented on code in PR #10678:
URL: https://github.com/apache/ozone/pull/10678#discussion_r3546282455
##########
hadoop-hdds/interface-client/src/main/proto/hdds.proto:
##########
@@ -433,6 +433,10 @@ message UpgradeStatus {
optional int32 numDatanodesFinalized = 2;
optional int32 numDatanodesTotal = 3;
optional bool shouldFinalize = 4;
+ optional uint32 scmSoftwareVersion = 5;
Review Comment:
This will only be the leader SCM's software version. Unlike apparent
version, these may not always match between SCMs. Maybe we should only return
apparent versions of all components with the verbose option, not software
version?
Note that software version can also be misleading if there was no component
version change across an upgrade. In that case all versions can match even if
the cluster is in mixed versions.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeManager.java:
##########
@@ -180,6 +182,14 @@ default DatanodeFinalizationCounts
getDatanodeFinalizationCounts() {
ComponentVersion dnApparentVersion =
datanodeInfo.getLastKnownApparentVersion();
ComponentVersion dnSoftwareVersion =
datanodeInfo.getLastKnownSoftwareVersion();
+ int dnApparentVersionInt = dnApparentVersion.serialize();
+ if (dnApparentVersionInt < minApparentVersion) {
+ minApparentVersion = dnApparentVersionInt;
+ }
+ if (dnApparentVersionInt > maxApparentVersion) {
+ maxApparentVersion = dnApparentVersionInt;
+ }
Review Comment:
nit.
```suggestion
minApparentVersion = Math.min(minApparentVersion,
dnApparentVersionInt);
maxApparentVersion = Math.max(maxApparentVersion,
dnApparentVersionInt);
```
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/upgrade/StatusSubCommand.java:
##########
@@ -51,20 +60,150 @@ public Integer call() throws Exception {
"`ozone admin scm finalizationstatus` and `ozone admin om
finalizationstatus`");
return 1;
}
- OzoneManagerProtocolProtos.QueryUpgradeStatusResponse status =
client.queryUpgradeStatus();
-
+ QueryUpgradeStatusResponse status = client.queryUpgradeStatus();
- out().println("Upgrade status:");
- out().println(" OM Finalized? " + status.getOmFinalized());
- out().println(" SCM Finalized? " +
status.getHddsStatus().getScmFinalized());
- out().println(" Datanodes finalized: " +
status.getHddsStatus().getNumDatanodesFinalized()
- + "/" + status.getHddsStatus().getNumDatanodesTotal());
+ if (json) {
+
out().println(JsonUtils.toJsonStringWithDefaultPrettyPrinter(UpgradeStatusDto.from(status)));
+ } else if (isVerbose()) {
+ printVerbose(status, out());
+ } else {
+ printBasic(status, out());
+ }
}
return 0;
}
+ /** Basic, non-verbose human-readable status. */
+ static void printBasic(QueryUpgradeStatusResponse status, PrintWriter out) {
+ out.println("Upgrade status:");
+ out.println(" OM Finalized? " + status.getOmFinalized());
+ out.println(" SCM Finalized? " +
status.getHddsStatus().getScmFinalized());
+ out.println(" Datanodes finalized: " +
status.getHddsStatus().getNumDatanodesFinalized()
+ + "/" + status.getHddsStatus().getNumDatanodesTotal());
+ }
+
+ /**
+ * Verbose human-readable status that includes the software/apparent
versions reported by OM
+ * and SCM, and the range of apparent versions across healthy datanodes
(last known from heartbeat).
+ * Version fields that are not populated by the server (e.g. when running
against an older
+ * cluster that does not yet ship these fields) are reported as
"(unavailable)".
+ */
+ static void printVerbose(QueryUpgradeStatusResponse status, PrintWriter out)
{
+ HddsProtos.UpgradeStatus hdds = status.getHddsStatus();
+ out.println("Upgrade status:");
+ out.println(" OM Finalized? " + status.getOmFinalized());
+ out.println(" OM Software Version: " +
formatOmVersion(status.hasOmSoftwareVersion(),
+ status.getOmSoftwareVersion()));
+ out.println(" OM Apparent Version: " +
formatOmVersion(status.hasOmApparentVersion(),
+ status.getOmApparentVersion()));
+ out.println(" SCM Finalized? " + hdds.getScmFinalized());
+ out.println(" SCM Software Version: " +
formatHddsVersion(hdds.hasScmSoftwareVersion(),
+ hdds.getScmSoftwareVersion()));
+ out.println(" SCM Apparent Version: " +
formatHddsVersion(hdds.hasScmApparentVersion(),
+ hdds.getScmApparentVersion()));
+ out.println(" Datanodes finalized: " +
hdds.getNumDatanodesFinalized() + "/" + hdds.getNumDatanodesTotal());
+ out.println(" Min DN Apparent Version: " +
formatHddsVersion(hdds.hasMinDatanodeApparentVersion(),
Review Comment:
```suggestion
out.println(" Min Datanode Apparent Version: " +
formatHddsVersion(hdds.hasMinDatanodeApparentVersion(),
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeManager.java:
##########
@@ -507,5 +524,13 @@ public int getTotalHealthyDatanodes() {
public boolean allNodesFinalized() {
return numFinalizedDatanodes == totalHealthyDatanodes;
}
+
+ public Integer getMinApparentVersion() {
Review Comment:
Why Integer object instead of int primitive for these getters?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeManager.java:
##########
@@ -194,7 +204,8 @@ default DatanodeFinalizationCounts
getDatanodeFinalizationCounts() {
}
}
- return new DatanodeFinalizationCounts(finalizedNodes, totalHealthyNodes);
+ return new DatanodeFinalizationCounts(finalizedNodes, totalHealthyNodes,
+ minApparentVersion, maxApparentVersion);
Review Comment:
Optional, but might be better to make this a builder so all the int
parameters don't accidentally get swapped.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/upgrade/StatusSubCommand.java:
##########
@@ -51,20 +60,150 @@ public Integer call() throws Exception {
"`ozone admin scm finalizationstatus` and `ozone admin om
finalizationstatus`");
return 1;
}
- OzoneManagerProtocolProtos.QueryUpgradeStatusResponse status =
client.queryUpgradeStatus();
-
+ QueryUpgradeStatusResponse status = client.queryUpgradeStatus();
- out().println("Upgrade status:");
- out().println(" OM Finalized? " + status.getOmFinalized());
- out().println(" SCM Finalized? " +
status.getHddsStatus().getScmFinalized());
- out().println(" Datanodes finalized: " +
status.getHddsStatus().getNumDatanodesFinalized()
- + "/" + status.getHddsStatus().getNumDatanodesTotal());
+ if (json) {
+
out().println(JsonUtils.toJsonStringWithDefaultPrettyPrinter(UpgradeStatusDto.from(status)));
+ } else if (isVerbose()) {
+ printVerbose(status, out());
+ } else {
+ printBasic(status, out());
+ }
}
return 0;
}
+ /** Basic, non-verbose human-readable status. */
+ static void printBasic(QueryUpgradeStatusResponse status, PrintWriter out) {
+ out.println("Upgrade status:");
+ out.println(" OM Finalized? " + status.getOmFinalized());
+ out.println(" SCM Finalized? " +
status.getHddsStatus().getScmFinalized());
+ out.println(" Datanodes finalized: " +
status.getHddsStatus().getNumDatanodesFinalized()
+ + "/" + status.getHddsStatus().getNumDatanodesTotal());
+ }
+
+ /**
+ * Verbose human-readable status that includes the software/apparent
versions reported by OM
+ * and SCM, and the range of apparent versions across healthy datanodes
(last known from heartbeat).
+ * Version fields that are not populated by the server (e.g. when running
against an older
+ * cluster that does not yet ship these fields) are reported as
"(unavailable)".
+ */
+ static void printVerbose(QueryUpgradeStatusResponse status, PrintWriter out)
{
+ HddsProtos.UpgradeStatus hdds = status.getHddsStatus();
+ out.println("Upgrade status:");
+ out.println(" OM Finalized? " + status.getOmFinalized());
+ out.println(" OM Software Version: " +
formatOmVersion(status.hasOmSoftwareVersion(),
+ status.getOmSoftwareVersion()));
+ out.println(" OM Apparent Version: " +
formatOmVersion(status.hasOmApparentVersion(),
+ status.getOmApparentVersion()));
+ out.println(" SCM Finalized? " + hdds.getScmFinalized());
+ out.println(" SCM Software Version: " +
formatHddsVersion(hdds.hasScmSoftwareVersion(),
+ hdds.getScmSoftwareVersion()));
+ out.println(" SCM Apparent Version: " +
formatHddsVersion(hdds.hasScmApparentVersion(),
+ hdds.getScmApparentVersion()));
+ out.println(" Datanodes finalized: " +
hdds.getNumDatanodesFinalized() + "/" + hdds.getNumDatanodesTotal());
+ out.println(" Min DN Apparent Version: " +
formatHddsVersion(hdds.hasMinDatanodeApparentVersion(),
+ hdds.getMinDatanodeApparentVersion()));
+ out.println(" Max DN Apparent Version: " +
formatHddsVersion(hdds.hasMaxDatanodeApparentVersion(),
+ hdds.getMaxDatanodeApparentVersion()));
+ }
+
+ private static String formatOmVersion(boolean present, int serialized) {
Review Comment:
We already have a server version check at the top of this command, so by the
time we get here we can assume that all fields are present according to the ZDU
supported server's contract.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/upgrade/StatusSubCommand.java:
##########
@@ -51,20 +60,150 @@ public Integer call() throws Exception {
"`ozone admin scm finalizationstatus` and `ozone admin om
finalizationstatus`");
return 1;
}
- OzoneManagerProtocolProtos.QueryUpgradeStatusResponse status =
client.queryUpgradeStatus();
-
+ QueryUpgradeStatusResponse status = client.queryUpgradeStatus();
- out().println("Upgrade status:");
- out().println(" OM Finalized? " + status.getOmFinalized());
- out().println(" SCM Finalized? " +
status.getHddsStatus().getScmFinalized());
- out().println(" Datanodes finalized: " +
status.getHddsStatus().getNumDatanodesFinalized()
- + "/" + status.getHddsStatus().getNumDatanodesTotal());
+ if (json) {
+
out().println(JsonUtils.toJsonStringWithDefaultPrettyPrinter(UpgradeStatusDto.from(status)));
+ } else if (isVerbose()) {
+ printVerbose(status, out());
+ } else {
+ printBasic(status, out());
+ }
}
return 0;
}
+ /** Basic, non-verbose human-readable status. */
+ static void printBasic(QueryUpgradeStatusResponse status, PrintWriter out) {
+ out.println("Upgrade status:");
+ out.println(" OM Finalized? " + status.getOmFinalized());
+ out.println(" SCM Finalized? " +
status.getHddsStatus().getScmFinalized());
+ out.println(" Datanodes finalized: " +
status.getHddsStatus().getNumDatanodesFinalized()
+ + "/" + status.getHddsStatus().getNumDatanodesTotal());
+ }
+
+ /**
+ * Verbose human-readable status that includes the software/apparent
versions reported by OM
+ * and SCM, and the range of apparent versions across healthy datanodes
(last known from heartbeat).
+ * Version fields that are not populated by the server (e.g. when running
against an older
+ * cluster that does not yet ship these fields) are reported as
"(unavailable)".
+ */
+ static void printVerbose(QueryUpgradeStatusResponse status, PrintWriter out)
{
+ HddsProtos.UpgradeStatus hdds = status.getHddsStatus();
+ out.println("Upgrade status:");
+ out.println(" OM Finalized? " + status.getOmFinalized());
+ out.println(" OM Software Version: " +
formatOmVersion(status.hasOmSoftwareVersion(),
+ status.getOmSoftwareVersion()));
+ out.println(" OM Apparent Version: " +
formatOmVersion(status.hasOmApparentVersion(),
+ status.getOmApparentVersion()));
+ out.println(" SCM Finalized? " + hdds.getScmFinalized());
+ out.println(" SCM Software Version: " +
formatHddsVersion(hdds.hasScmSoftwareVersion(),
+ hdds.getScmSoftwareVersion()));
+ out.println(" SCM Apparent Version: " +
formatHddsVersion(hdds.hasScmApparentVersion(),
+ hdds.getScmApparentVersion()));
+ out.println(" Datanodes finalized: " +
hdds.getNumDatanodesFinalized() + "/" + hdds.getNumDatanodesTotal());
+ out.println(" Min DN Apparent Version: " +
formatHddsVersion(hdds.hasMinDatanodeApparentVersion(),
+ hdds.getMinDatanodeApparentVersion()));
+ out.println(" Max DN Apparent Version: " +
formatHddsVersion(hdds.hasMaxDatanodeApparentVersion(),
Review Comment:
```suggestion
out.println(" Max Datanode Apparent Version: " +
formatHddsVersion(hdds.hasMaxDatanodeApparentVersion(),
```
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/ozone/admin/upgrade/TestFinalizeSubCommand.java:
##########
@@ -119,6 +130,152 @@ public void
testNonZduServerPrintsErrorAndReturnsNonZero() throws Exception {
verify(omClient, never()).finalizeUpgrade();
}
+ @Test
+ public void testWithoutWaitFlagDoesNotPollStatus() throws Exception {
+ new CommandLine(cmd).parseArgs();
+ assertEquals(0, cmd.call());
+
+ verify(omClient, never()).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagCompletesImmediatelyWhenAlreadyFinalized() throws
Exception {
+ when(omClient.queryUpgradeStatus()).thenReturn(finalizedStatus(3, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+ assertEquals(0, cmd.call());
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(output.contains("Finalization complete."));
+ verify(omClient).finalizeUpgrade();
+ verify(omClient, times(1)).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagPollsUntilFinalized() throws Exception {
+ when(omClient.queryUpgradeStatus())
+ .thenReturn(inProgressStatus(0, 3))
+ .thenReturn(inProgressStatus(2, 3))
+ .thenReturn(finalizedStatus(3, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+ assertEquals(0, cmd.call());
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(output.contains("Waiting for finalization"));
+ assertTrue(output.contains("Finalization complete."));
+ verify(omClient, times(3)).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagInterruptIsHandledCleanly() throws Exception {
+ // Make the poll interval long enough that the interrupt lands during
sleep.
+ cmd.setPollIntervalMillis(60_000);
+ when(omClient.queryUpgradeStatus()).thenReturn(inProgressStatus(0, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+
+ Thread runner = new Thread(() -> {
+ try {
+ cmd.call();
+ } catch (Exception ignored) {
+ // assertion in main thread
+ }
+ });
+ runner.start();
+ // Give the runner a moment to start sleeping.
+ Thread.sleep(50);
+ runner.interrupt();
+ runner.join(5_000);
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(output.contains("Waiting interrupted"));
+ verify(omClient, never()).queryUpgradeStatus();
Review Comment:
Once we remove the initial delay in the polling this should call the query
command once.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/upgrade/StatusSubCommand.java:
##########
@@ -51,20 +60,150 @@ public Integer call() throws Exception {
"`ozone admin scm finalizationstatus` and `ozone admin om
finalizationstatus`");
return 1;
}
- OzoneManagerProtocolProtos.QueryUpgradeStatusResponse status =
client.queryUpgradeStatus();
-
+ QueryUpgradeStatusResponse status = client.queryUpgradeStatus();
- out().println("Upgrade status:");
- out().println(" OM Finalized? " + status.getOmFinalized());
- out().println(" SCM Finalized? " +
status.getHddsStatus().getScmFinalized());
- out().println(" Datanodes finalized: " +
status.getHddsStatus().getNumDatanodesFinalized()
- + "/" + status.getHddsStatus().getNumDatanodesTotal());
+ if (json) {
+
out().println(JsonUtils.toJsonStringWithDefaultPrettyPrinter(UpgradeStatusDto.from(status)));
+ } else if (isVerbose()) {
+ printVerbose(status, out());
+ } else {
+ printBasic(status, out());
+ }
}
return 0;
}
+ /** Basic, non-verbose human-readable status. */
+ static void printBasic(QueryUpgradeStatusResponse status, PrintWriter out) {
+ out.println("Upgrade status:");
+ out.println(" OM Finalized? " + status.getOmFinalized());
+ out.println(" SCM Finalized? " +
status.getHddsStatus().getScmFinalized());
+ out.println(" Datanodes finalized: " +
status.getHddsStatus().getNumDatanodesFinalized()
+ + "/" + status.getHddsStatus().getNumDatanodesTotal());
+ }
+
+ /**
+ * Verbose human-readable status that includes the software/apparent
versions reported by OM
+ * and SCM, and the range of apparent versions across healthy datanodes
(last known from heartbeat).
+ * Version fields that are not populated by the server (e.g. when running
against an older
+ * cluster that does not yet ship these fields) are reported as
"(unavailable)".
+ */
+ static void printVerbose(QueryUpgradeStatusResponse status, PrintWriter out)
{
+ HddsProtos.UpgradeStatus hdds = status.getHddsStatus();
+ out.println("Upgrade status:");
+ out.println(" OM Finalized? " + status.getOmFinalized());
+ out.println(" OM Software Version: " +
formatOmVersion(status.hasOmSoftwareVersion(),
+ status.getOmSoftwareVersion()));
+ out.println(" OM Apparent Version: " +
formatOmVersion(status.hasOmApparentVersion(),
+ status.getOmApparentVersion()));
+ out.println(" SCM Finalized? " + hdds.getScmFinalized());
+ out.println(" SCM Software Version: " +
formatHddsVersion(hdds.hasScmSoftwareVersion(),
+ hdds.getScmSoftwareVersion()));
+ out.println(" SCM Apparent Version: " +
formatHddsVersion(hdds.hasScmApparentVersion(),
+ hdds.getScmApparentVersion()));
+ out.println(" Datanodes finalized: " +
hdds.getNumDatanodesFinalized() + "/" + hdds.getNumDatanodesTotal());
+ out.println(" Min DN Apparent Version: " +
formatHddsVersion(hdds.hasMinDatanodeApparentVersion(),
+ hdds.getMinDatanodeApparentVersion()));
+ out.println(" Max DN Apparent Version: " +
formatHddsVersion(hdds.hasMaxDatanodeApparentVersion(),
+ hdds.getMaxDatanodeApparentVersion()));
+ }
+
+ private static String formatOmVersion(boolean present, int serialized) {
Review Comment:
I think we should still use the DTO below to decouple the json output from
the proto format, but that can be simplified along these lines as well.
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/ozone/admin/upgrade/TestFinalizeSubCommand.java:
##########
@@ -119,6 +130,152 @@ public void
testNonZduServerPrintsErrorAndReturnsNonZero() throws Exception {
verify(omClient, never()).finalizeUpgrade();
}
+ @Test
+ public void testWithoutWaitFlagDoesNotPollStatus() throws Exception {
+ new CommandLine(cmd).parseArgs();
+ assertEquals(0, cmd.call());
+
+ verify(omClient, never()).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagCompletesImmediatelyWhenAlreadyFinalized() throws
Exception {
+ when(omClient.queryUpgradeStatus()).thenReturn(finalizedStatus(3, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+ assertEquals(0, cmd.call());
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(output.contains("Finalization complete."));
+ verify(omClient).finalizeUpgrade();
+ verify(omClient, times(1)).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagPollsUntilFinalized() throws Exception {
+ when(omClient.queryUpgradeStatus())
+ .thenReturn(inProgressStatus(0, 3))
+ .thenReturn(inProgressStatus(2, 3))
+ .thenReturn(finalizedStatus(3, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+ assertEquals(0, cmd.call());
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(output.contains("Waiting for finalization"));
+ assertTrue(output.contains("Finalization complete."));
+ verify(omClient, times(3)).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagInterruptIsHandledCleanly() throws Exception {
+ // Make the poll interval long enough that the interrupt lands during
sleep.
+ cmd.setPollIntervalMillis(60_000);
+ when(omClient.queryUpgradeStatus()).thenReturn(inProgressStatus(0, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+
+ Thread runner = new Thread(() -> {
+ try {
+ cmd.call();
+ } catch (Exception ignored) {
+ // assertion in main thread
Review Comment:
We can still make sure this failed with the correct result, either by
checking the exception type or the thread's interrupt flag.
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/ozone/admin/upgrade/TestFinalizeSubCommand.java:
##########
@@ -119,6 +130,152 @@ public void
testNonZduServerPrintsErrorAndReturnsNonZero() throws Exception {
verify(omClient, never()).finalizeUpgrade();
}
+ @Test
+ public void testWithoutWaitFlagDoesNotPollStatus() throws Exception {
+ new CommandLine(cmd).parseArgs();
+ assertEquals(0, cmd.call());
+
+ verify(omClient, never()).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagCompletesImmediatelyWhenAlreadyFinalized() throws
Exception {
+ when(omClient.queryUpgradeStatus()).thenReturn(finalizedStatus(3, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+ assertEquals(0, cmd.call());
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(output.contains("Finalization complete."));
+ verify(omClient).finalizeUpgrade();
+ verify(omClient, times(1)).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagPollsUntilFinalized() throws Exception {
+ when(omClient.queryUpgradeStatus())
+ .thenReturn(inProgressStatus(0, 3))
+ .thenReturn(inProgressStatus(2, 3))
+ .thenReturn(finalizedStatus(3, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+ assertEquals(0, cmd.call());
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(output.contains("Waiting for finalization"));
+ assertTrue(output.contains("Finalization complete."));
+ verify(omClient, times(3)).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagInterruptIsHandledCleanly() throws Exception {
+ // Make the poll interval long enough that the interrupt lands during
sleep.
+ cmd.setPollIntervalMillis(60_000);
+ when(omClient.queryUpgradeStatus()).thenReturn(inProgressStatus(0, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+
+ Thread runner = new Thread(() -> {
+ try {
+ cmd.call();
+ } catch (Exception ignored) {
+ // assertion in main thread
+ }
+ });
+ runner.start();
+ // Give the runner a moment to start sleeping.
+ Thread.sleep(50);
+ runner.interrupt();
+ runner.join(5_000);
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(output.contains("Waiting interrupted"));
+ verify(omClient, never()).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagIsResumableAfterCancel() throws Exception {
+ // First invocation: block forever in Thread.sleep so we can interrupt.
+ cmd.setPollIntervalMillis(60_000);
+ // Second invocation is expected to see a finalized cluster on the first
poll.
+ when(omClient.queryUpgradeStatus()).thenReturn(finalizedStatus(3, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+
+ Thread runner = new Thread(() -> {
+ try {
+ cmd.call();
+ } catch (Exception ignored) {
+ // First call swallows interrupt cleanly; nothing to assert from the
side thread.
+ }
+ });
+ runner.start();
+ Thread.sleep(50);
+ runner.interrupt();
+ runner.join(5_000);
+
+ String firstOutput = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(firstOutput.contains("Cluster finalization has been started"));
+ assertTrue(firstOutput.contains("Waiting interrupted"));
+ // Side thread was interrupted during the initial sleep, so the first poll
never happened.
+ verify(omClient, never()).queryUpgradeStatus();
+
+ // Second invocation — reset output capture and shrink the poll interval
so it exits promptly.
+ outContent.reset();
+ cmd.setPollIntervalMillis(1);
+
+ new CommandLine(cmd).parseArgs("--wait");
+ assertEquals(0, cmd.call());
+
+ String secondOutput = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(secondOutput.contains("Cluster finalization has been started"));
+ assertTrue(secondOutput.contains("Finalization complete."));
+ // finalizeUpgrade must have been issued on both runs (idempotent
server-side).
+ verify(omClient, times(2)).finalizeUpgrade();
+ // queryUpgradeStatus only ran during the second invocation.
+ verify(omClient, times(1)).queryUpgradeStatus();
+ // The client is closed after each try-with-resources block.
+ verify(omClient, times(2)).close();
+ }
+
+ @Test
+ public void testWaitFlagWithVerbosePrintsFullStatus() throws Exception {
+ verbose = true;
+ when(omClient.queryUpgradeStatus())
+ .thenReturn(finalizedStatus(2, 2));
+
+ new CommandLine(cmd).parseArgs("--wait");
+ assertEquals(0, cmd.call());
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ // Verbose output uses the StatusSubCommand.printVerbose layout.
Review Comment:
I don't see any checks on verbose specific output here.
##########
hadoop-hdds/interface-client/src/main/proto/hdds.proto:
##########
@@ -433,6 +433,10 @@ message UpgradeStatus {
optional int32 numDatanodesFinalized = 2;
optional int32 numDatanodesTotal = 3;
optional bool shouldFinalize = 4;
+ optional uint32 scmSoftwareVersion = 5;
+ optional uint32 scmApparentVersion = 6;
+ optional uint32 minDatanodeApparentVersion = 7;
+ optional uint32 maxDatanodeApparentVersion = 8;
Review Comment:
In the OM proto we are using int32 for the version type. I don't have a
strong preference but should keep them consistent.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/OMUpgradeTestUtils.java:
##########
@@ -71,12 +71,14 @@ public static void waitForFinalization(OzoneManagerProtocol
omClient)
throws TimeoutException, InterruptedException {
waitFor(() -> {
try {
- UpgradeFinalization.StatusAndMessages statusAndMessages =
- omClient.queryUpgradeFinalizationProgress("finalize-test", false,
- false);
- System.out.println("Finalization Messages : " +
- statusAndMessages.msgs());
- return statusAndMessages.status().equals(FINALIZATION_DONE);
+ QueryUpgradeStatusResponse status = omClient.queryUpgradeStatus();
+ HddsProtos.UpgradeStatus hdds = status.getHddsStatus();
+ System.out.println("Finalization status: omFinalized=" +
status.getOmFinalized()
Review Comment:
Since this is called during tests we should use a logger here.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeManager.java:
##########
@@ -154,6 +154,8 @@ default int getAllNodeCount() {
default DatanodeFinalizationCounts getDatanodeFinalizationCounts() {
int finalizedNodes = 0;
int totalHealthyNodes = 0;
+ int minApparentVersion = 0;
Review Comment:
This should either be initialized to `Integer.MAX_VALUE` or the first
concrete DN version we get. Currently the min will always be reported as zero.
Looks like there's a unit test gap for these SCM changes.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/upgrade/FinalizeSubCommand.java:
##########
@@ -51,11 +65,65 @@ public Integer call() throws Exception {
}
client.finalizeUpgrade();
out().println("Cluster finalization has been started. Monitor progress
with `ozone admin upgrade status`");
+
+ if (wait) {
+ return waitForFinalization(client);
+ }
}
return 0;
}
+ /**
+ * Polls the cluster status until OM, SCM and all healthy datanodes report
finalized, the operator
+ * interrupts the command, or an RPC fails. {@code --wait} is safe to
re-run: if the cluster is already
+ * finalized, the first poll returns done and the command exits 0.
+ */
+ private int waitForFinalization(OzoneManagerProtocol client) {
+ while (true) {
+ try {
+ Thread.sleep(pollIntervalMillis);
Review Comment:
We should check first then sleep to avoid waiting 5 seconds even when the
cluster is already finalized.
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/ozone/admin/upgrade/TestFinalizeSubCommand.java:
##########
@@ -119,6 +130,152 @@ public void
testNonZduServerPrintsErrorAndReturnsNonZero() throws Exception {
verify(omClient, never()).finalizeUpgrade();
}
+ @Test
+ public void testWithoutWaitFlagDoesNotPollStatus() throws Exception {
+ new CommandLine(cmd).parseArgs();
+ assertEquals(0, cmd.call());
+
+ verify(omClient, never()).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagCompletesImmediatelyWhenAlreadyFinalized() throws
Exception {
Review Comment:
The test does not actually check that the poll interval is not used when the
cluster is already finalized. In the current version the wait flag actually
doesn't complete immediately when the cluster is already finalized: it still
waits 5 seconds.
```suggestion
public void testStatusCalledOnceWhenAlreadyFinalized() throws Exception {
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
Review Comment:
We should block this query when SCM is in safemode just like the finalize
command. This way we don't have to handle the zero datanode case.
--
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]
