HDDS-14940. Create skeleton for ozone admin upgrade status command#10011
HDDS-14940. Create skeleton for ozone admin upgrade status command#10011sodonnel wants to merge 2 commits intoapache:HDDS-14496-zdufrom
Conversation
github-actions
bot
added
the
zdu
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @sodonnel for the patch.
| required bool scmFinalized = 1; | ||
| required int32 numDatanodesFinalized = 2; | ||
| required int32 numDatanodesTotal = 3; | ||
| required bool shouldFinalize = 4; |
There was a problem hiding this comment.
Please add new fields as optional.
https://protobuf.dev/programming-guides/proto2/#required-deprecated
https://protobuf.dev/programming-guides/style/#required
There was a problem hiding this comment.
This is a new object - shouldn't all its fields be required as we have no backward compatibility issue here - nothing else can be using this object?
There was a problem hiding this comment.
It's about backward compatibility of a future version of the software. From the docs I linked:
requiredmust not be used for new fields. Semantics for required fields should be implemented at the application layer instead
There was a problem hiding this comment.
OK - this probably should be communicated more widely, as it seems that the established pattern is now not to be followed, and I suspect many people don't know this. The current definition file is also full of required fields.
| System.out.println("Update status:"); | ||
| System.out.println(" SCM Finalized: " + status.getScmFinalized()); | ||
| System.out.println(" Datanodes finalized: " + status.getNumDatanodesFinalized()); | ||
| System.out.println(" Total Datanodes: " + status.getNumDatanodesTotal()); | ||
| System.out.println(" Should Finalize: " + status.getShouldFinalize()); |
There was a problem hiding this comment.
nit: please use out() (inherited from AbstractSubcommand) instead of System.out.
| */ | ||
| @CommandLine.Command( | ||
| name = "status", | ||
| description = "Upgrade status of the cluster", |
There was a problem hiding this comment.
nit:
| description = "Upgrade status of the cluster", | |
| description = "Show status of cluster upgrade", |
| */ | ||
| @CommandLine.Command( | ||
| name = "upgrade", | ||
| description = "Upgrade specific operations", |
There was a problem hiding this comment.
nit:
| description = "Upgrade specific operations", | |
| description = "Operations related to cluster upgrade", |
| return HddsProtos.UpgradeStatus.newBuilder() | ||
| .setScmFinalized(true) | ||
| .setNumDatanodesFinalized(10) | ||
| .setNumDatanodesTotal(10) | ||
| .setShouldFinalize(true) | ||
| .build(); | ||
| } catch (IOException ex) { | ||
| AUDIT.logReadFailure(buildAuditMessageForFailure(SCMAction.QUERY_UPGRADE_STATUS, auditMap, ex)); |
There was a problem hiding this comment.
AUDIT.logReadSuccess is missing. Should be something like:
HddsProtos.UpgradeStatus response = ...
AUDIT.logReadSuccess(...)
return response2 participants
What changes were proposed in this pull request?
Create the skeleton for a new upgrade command ozone admin upgrade status, which will eventually replace the existing commands. This initial version will connect to SCM and pull a placeholder response. The logic to actually query the SCM status will be added in followup PRs.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14940
How was this patch tested?
Tested manually in docker to prove the command is wired up correctly. Tests can be added when we have real functionality in the command to test.