HDDS-14940. Create skeleton for ozone admin upgrade status command#10011
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", |
errose28
left a comment
errose28
left a comment
There was a problem hiding this comment.
LGTM if we remove the parameters from queryUpgradeStatus since they won't be required in the new model.
| String upgradeClientID, boolean force, boolean readonly) | ||
| throws IOException; | ||
|
|
||
| HddsProtos.UpgradeStatus queryUpgradeStatus(String upgradeClientID, boolean readonly) throws IOException; |
There was a problem hiding this comment.
We don't need client ID and readonly parameters for the new API. Finalization will not be tracking message state per client now that is one atomic operation per component.
| // Temporary output to validate the command is working. | ||
| out().println("Update status:"); | ||
| out().println(" SCM Finalized: " + status.getScmFinalized()); | ||
| out().println(" Datanodes finalized: " + status.getNumDatanodesFinalized()); | ||
| out().println(" Total Datanodes: " + status.getNumDatanodesTotal()); | ||
| out().println(" Should Finalize: " + status.getShouldFinalize()); |
There was a problem hiding this comment.
This is fine for now. I'm thinking the final output would have a "human" version:
OM is not finalized
SCM is finalized
3/10 Datanodes are finalized
and then a --json version for scripting:
{
"omFinalized": false,
"scmFinalized": true,
"finalizedDatanodes": 3,
"totalDatanodes": 10
}Note that the final version of this command will also query OM for its finalization status.
We also don't need to print the shouldFinalize parameter. That is for SCM to explicitly tell OM when it should finalize so OM does not need to infer this from datanode counts.
There was a problem hiding this comment.
I vote +1 on the human and JSON versions.
If we don't print the shouldFinalize parameter though (and probably even if we do), it would make a lot of sense to log it when it changes. That would be useful for troubleshooting.
There was a problem hiding this comment.
Yea the JSON can final output in human readable for can be finalized later when we have the OM status included too. This change was just to get the basic path through the system setup so we can build on it with further small changes.
|
Same here - the change lines up with what we talked about, it prints the expected content, and the new subcommands appear in the CLI help. I had one thought inline about printing |
|
I'm good with this change with Zita's latest commit - we can tweak the output and add the JSON as we continue to develop it. I cannot really give a +1 myself as I am the part author of the change, but I am +1 to commit it. |
errose28
left a comment
errose28
left a comment
There was a problem hiding this comment.
Thanks for the updates. We can continue to build off this in follow-up changes.
* HDDS-14496-zdu: HDDS-14940. Create skeleton for ozone admin upgrade status command (apache#10011)
5 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.