HDDS-15716. Replace layout version protos with new version-named protos in the DN–SCM protocol#10817
HDDS-15716. Replace layout version protos with new version-named protos in the DN–SCM protocol#10817dombizita wants to merge 2 commits into
Conversation
…os in the DN–SCM protocol
github-actions
Bot
added
the
zdu
errose28
left a comment
errose28
left a comment
There was a problem hiding this comment.
Thanks for adding this @dombizita. Most comments are minor, but there's some improvements we can do to existing behavior as well.
| * Replaces LayoutVersionProto, using the versioning terminology of | ||
| * ComponentVersionManager: apparentVersion and softwareVersion. |
There was a problem hiding this comment.
| * Replaces LayoutVersionProto, using the versioning terminology of | |
| * ComponentVersionManager: apparentVersion and softwareVersion. | |
| * Used by Datanodes to report their versions to SCM. |
| * | ||
| * Deprecated, please use DatanodeVersionProto instead. The field names | ||
| * metadataLayoutVersion and softwareLayoutVersion have been replaced by | ||
| * apparentVersion and softwareVersion respectively. |
There was a problem hiding this comment.
Let's make the deprecation message the first thing in the comment.
| * Replaces FinalizeNewLayoutVersionCommandProto. | ||
| */ | ||
| message FinalizeNewDatanodeVersionCommandProto { | ||
| required bool finalizeNewDatanodeVersion = 1 [default = false]; |
There was a problem hiding this comment.
This field is not needed, even in the old version. Sending the command is enough to instruct the DN to finalize without needing a separate flag to make it active.
| message FinalizeNewDatanodeVersionCommandProto { | ||
| required bool finalizeNewDatanodeVersion = 1 [default = false]; | ||
| optional DatanodeVersionProto datanodeVersion = 2; | ||
| required int64 cmdId = 3; |
There was a problem hiding this comment.
Other commands have this required but for new fields we should be using optional for everything, so IMO this should be optional too.
| /** | ||
| * This command asks the DataNode to finalize a new layout version. | ||
| * | ||
| * Deprecated, please use FinalizeNewDatanodeVersionCommandProto instead, which |
There was a problem hiding this comment.
Again let's make the deprecation message the first thing in the comment.
| * This command asks the DataNode to finalize a new datanode version. | ||
| * Replaces FinalizeNewLayoutVersionCommandProto. |
There was a problem hiding this comment.
IMO better to have the deprecated command point forward to the new one than the new one point back to the deprecated one.
| * This command asks the DataNode to finalize a new datanode version. | |
| * Replaces FinalizeNewLayoutVersionCommandProto. | |
| * This command asks the Datanode to finalize a new datanode version. |
| ((FinalizeVersionCommand) command).getProto(); | ||
| try { | ||
| if (finalizeCommand.getFinalizeNewLayoutVersion()) { | ||
| if (finalizeCommand.getFinalizeNewDatanodeVersion()) { |
There was a problem hiding this comment.
We should probably add a check that DN and SCM software versions match here, the open question is what to do about it. I'm currently thinking we should crash the node, since there's already safeguards checking versions when the DN registers and before SCM finalizes itself. With those in place it shouldn't even be possible to hit this code block so I think we can be defensive.
| */ | ||
| message FinalizeNewDatanodeVersionCommandProto { | ||
| required bool finalizeNewDatanodeVersion = 1 [default = false]; | ||
| optional DatanodeVersionProto datanodeVersion = 2; |
There was a problem hiding this comment.
I know the old command had this, but it's not intuitive what is supposed to go here. Is it for SCM to send the expected datanode software and apparent version it wants this node to be on? In that case the values will always be the same. I'm wondering if we should just replace this with an expected software version field so the DN can double check before going through with finalization.
| */ | ||
| public static class LayoutReportFromDatanode | ||
| extends ReportFromDatanode<LayoutVersionProto> { | ||
| public static class VersionReportFromDatanode |
There was a problem hiding this comment.
Looks like this is unused. I'm not sure what its original purpose was but we can remove it.
| LayoutVersionProto versionInfo = null; | ||
| if (!heartbeat.hasDataNodeLayoutVersion()) { | ||
| DatanodeVersionProto versionInfo = null; | ||
| if (!heartbeat.hasDatanodeVersion()) { |
There was a problem hiding this comment.
I don't know why this block was put here in the original framework. There's a similar one in StorageContainerDatanodeProtocolServerSideTranslatorPB#register. It only makes sense if you can ZDU between a version that doesn't support reporting versions and one that does, which has never been possible. This will still be true going forward since we have to use a non-rolling upgrade to get to this ZDU version first.
For registering, we can make SCMNodeManager#shouldFenceDatanode null safe so we get a node not permitted error when a node tries to register with no version info.
On heartbeats we can choose to call processVersionReport only when a version is attached. Technically it is the "last known version" so it doesn't need to be in every heartbeat, even though it is expected based on all existing datanode implementations.
There was a problem hiding this comment.
Both type changes in this file can have their corresponding variable renamed too.
2 participants
What changes were proposed in this pull request?
The current proto uses the old terminology in the names. We will have one non-rolling upgrade before ZDU is released where we can deprecate this proto and switch to a new one without any extra compatibility handling.
In this patch I switched
LayoutVersionPrototoDatanodeVersionProtoandFinalizeNewLayoutVersionCommandPrototoFinalizeNewDatanodeVersionCommandProto.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15716
How was this patch tested?
Green CI on my fork: https://github.com/dombizita/ozone/actions/runs/29577100863
Used Claude Opus 4.8 for the refactoring, review and adjustments by me.