adoroszlai commented on code in PR #9586:
URL: https://github.com/apache/ozone/pull/9586#discussion_r2661034734
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java:
##########
@@ -1049,12 +1049,24 @@ public ContainerBalancerStatusInfoResponseProto
getContainerBalancerStatusInfo()
ContainerBalancerStatusInfoRequestProto request =
ContainerBalancerStatusInfoRequestProto.getDefaultInstance();
- ContainerBalancerStatusInfoResponseProto response =
- submitRequest(Type.GetContainerBalancerStatusInfo,
- builder ->
builder.setContainerBalancerStatusInfoRequest(request))
- .getContainerBalancerStatusInfoResponse();
- return response;
-
+ try {
+ ContainerBalancerStatusInfoResponseProto response =
+ submitRequest(Type.GetContainerBalancerStatusInfo,
+ builder ->
builder.setContainerBalancerStatusInfoRequest(request))
+ .getContainerBalancerStatusInfoResponse();
+ return response;
+ } catch (IOException e) {
+ // HDDS-11120 - Added a rich rebalancing status info
+ // Backward compatibility fix - newer clients (2.0 >=) gracefully
fallback to the old
+ // API when connecting to older servers (< 2.0) that don't support the
new enum value.
+ if (e.getMessage() != null && e.getMessage().contains("missing required
fields")) {
Review Comment:
Can we check for the underlying `InvalidProtocolBufferException` instead?
I'm not sure the text message will always be the same.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java:
##########
@@ -1049,12 +1049,24 @@ public ContainerBalancerStatusInfoResponseProto
getContainerBalancerStatusInfo()
ContainerBalancerStatusInfoRequestProto request =
ContainerBalancerStatusInfoRequestProto.getDefaultInstance();
- ContainerBalancerStatusInfoResponseProto response =
- submitRequest(Type.GetContainerBalancerStatusInfo,
- builder ->
builder.setContainerBalancerStatusInfoRequest(request))
- .getContainerBalancerStatusInfoResponse();
- return response;
-
+ try {
+ ContainerBalancerStatusInfoResponseProto response =
+ submitRequest(Type.GetContainerBalancerStatusInfo,
+ builder ->
builder.setContainerBalancerStatusInfoRequest(request))
+ .getContainerBalancerStatusInfoResponse();
Review Comment:
nit: please reduce indent of "line continuation" from 8 to 4 while already
touching these lines.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java:
##########
@@ -59,7 +59,7 @@ public void execute(ScmClient scmClient) throws IOException {
ContainerBalancerStatusInfoResponseProto response =
scmClient.getContainerBalancerStatusInfo();
boolean isRunning = response.getIsRunning();
ContainerBalancerStatusInfoProto balancerStatusInfo =
response.getContainerBalancerStatusInfo();
- if (isRunning) {
+ if (isRunning && balancerStatusInfo != null) {
Instant startedAtInstant =
Instant.ofEpochSecond(balancerStatusInfo.getStartedAt());
LocalDateTime dateTime =
LocalDateTime.ofInstant(startedAtInstant, ZoneId.systemDefault());
Review Comment:
nit: Let's move these two statements inside `isVerbose()` (since values are
only used there), and add `balancerStatusInfo != null` check in that `if`.
Then we don't need a new outer `else if` and duplicate `println`.
--
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]
