errose28 commented on code in PR #10678:
URL: https://github.com/apache/ozone/pull/10678#discussion_r3640021087
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3664,11 +3665,27 @@ public void finalizeUpgrade() throws IOException {
@Override
public QueryUpgradeStatusResponse queryUpgradeStatus() throws IOException {
- HddsProtos.UpgradeStatus scmStatus =
scmClient.getContainerClient().queryUpgradeStatus();
+ HddsProtos.UpgradeStatus scmStatus;
+ try {
+ scmStatus = scmClient.getContainerClient().queryUpgradeStatus();
+ } catch (SCMException e) {
+ // SCM refuses the query while in safe mode
+ if (e.getResult() == SCMException.ResultCodes.SAFE_MODE_EXCEPTION) {
+ throw new OMException(e.getMessage(), e, NOT_SUPPORTED_OPERATION);
+ }
+ throw e;
+ }
+
+ boolean omFinalized = !versionManager.needsFinalization();
+ boolean clusterFinalized = omFinalized
+ && scmStatus.getScmFinalized()
+ && scmStatus.getNumDatanodesFinalized() ==
scmStatus.getNumDatanodesTotal();
Review Comment:
I think we should rename `shouldFinalize` to `hddsFinalized` in the proto
and change this to `omFinalized && hddsFinalized`. That is clearer about the
current state of the cluster, and doesn't duplicate the SCM finalization check
within the OM.
##########
hadoop-hdds/interface-client/src/main/proto/hdds.proto:
##########
@@ -434,6 +434,9 @@ message UpgradeStatus {
optional int32 numDatanodesFinalized = 2;
optional int32 numDatanodesTotal = 3;
optional bool shouldFinalize = 4;
+ optional uint32 scmApparentVersion = 5;
Review Comment:
Let's add a comment to the above `UpgradeFinalizationStatus` that it is kept
for compatibility with old clients, and that this proto is the currently active
one.
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1668,6 +1668,9 @@ message QueryUpgradeStatusRequest {
message QueryUpgradeStatusResponse {
optional bool omFinalized = 1;
optional hadoop.hdds.UpgradeStatus hddsStatus = 2;
+ optional uint32 omApparentVersion = 3;
+ // True when OM, SCM and all healthy datanodes are finalized
+ optional bool clusterFinalized = 4;
Review Comment:
One nice thing about a feature branch is we can still change the ordering
within protos unique to the branch. I put the OM fields last here so they will
still be next to each other if more like software version are added later.
```suggestion
// True when OM, SCM and all healthy datanodes are finalized
optional bool clusterFinalized = 1;
optional hadoop.hdds.UpgradeStatus hddsStatus = 2;
optional bool omFinalized = 3;
optional uint32 omApparentVersion = 4;
```
##########
hadoop-hdds/interface-client/src/main/proto/hdds.proto:
##########
@@ -434,6 +434,9 @@ message UpgradeStatus {
optional int32 numDatanodesFinalized = 2;
optional int32 numDatanodesTotal = 3;
optional bool shouldFinalize = 4;
+ optional uint32 scmApparentVersion = 5;
+ optional uint32 minDatanodeApparentVersion = 6;
+ optional uint32 maxDatanodeApparentVersion = 7;
Review Comment:
This is an extension of the comments on OM and its proto. Here I grouped the
fields by type instead of component since there is a mix of components in one
message.
```suggestion
optional bool hddsFinalized = 1;
optional bool scmFinalized = 2;
optional int32 numDatanodesFinalized = 3;
optional int32 numDatanodesTotal = 4;
optional uint32 scmApparentVersion = 5;
optional uint32 minDatanodeApparentVersion = 6;
optional uint32 maxDatanodeApparentVersion = 7;
```
--
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]
