errose28 commented on code in PR #10817:
URL: https://github.com/apache/ozone/pull/10817#discussion_r3616863323
##########
hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto:
##########
@@ -498,13 +515,26 @@ message ProcessCRLCommandProto {
/**
* This command asks the DataNode to finalize a new layout version.
+ *
+ * Deprecated, please use FinalizeNewDatanodeVersionCommandProto instead, which
+ * carries a DatanodeVersionProto instead of the deprecated LayoutVersionProto.
*/
message FinalizeNewLayoutVersionCommandProto {
required bool finalizeNewLayoutVersion = 1 [default = false];
required LayoutVersionProto dataNodeLayoutVersion = 2;
required int64 cmdId = 3;
}
+/**
+ * This command asks the DataNode to finalize a new datanode version.
+ * Replaces FinalizeNewLayoutVersionCommandProto.
+ */
+message FinalizeNewDatanodeVersionCommandProto {
+ required bool finalizeNewDatanodeVersion = 1 [default = false];
Review 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.
##########
hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto:
##########
@@ -498,13 +515,26 @@ message ProcessCRLCommandProto {
/**
* This command asks the DataNode to finalize a new layout version.
+ *
+ * Deprecated, please use FinalizeNewDatanodeVersionCommandProto instead, which
+ * carries a DatanodeVersionProto instead of the deprecated LayoutVersionProto.
*/
message FinalizeNewLayoutVersionCommandProto {
required bool finalizeNewLayoutVersion = 1 [default = false];
required LayoutVersionProto dataNodeLayoutVersion = 2;
required int64 cmdId = 3;
}
+/**
+ * This command asks the DataNode to finalize a new datanode version.
+ * Replaces FinalizeNewLayoutVersionCommandProto.
+ */
+message FinalizeNewDatanodeVersionCommandProto {
+ required bool finalizeNewDatanodeVersion = 1 [default = false];
+ optional DatanodeVersionProto datanodeVersion = 2;
+ required int64 cmdId = 3;
Review Comment:
Other commands have this required but for new fields we should be using
optional for everything, so IMO this should be optional too.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/FinalizeVersionCommandHandler.java:
##########
@@ -69,10 +69,10 @@ public void handle(SCMCommand<?> command, OzoneContainer
ozoneContainer,
invocationCount.incrementAndGet();
final long startTime = Time.monotonicNow();
DatanodeStateMachine dsm = context.getParent();
- final FinalizeNewLayoutVersionCommandProto finalizeCommand =
+ final FinalizeNewDatanodeVersionCommandProto finalizeCommand =
((FinalizeVersionCommand) command).getProto();
try {
- if (finalizeCommand.getFinalizeNewLayoutVersion()) {
+ if (finalizeCommand.getFinalizeNewDatanodeVersion()) {
Review 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.
##########
hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto:
##########
@@ -498,13 +515,26 @@ message ProcessCRLCommandProto {
/**
* This command asks the DataNode to finalize a new layout version.
+ *
+ * Deprecated, please use FinalizeNewDatanodeVersionCommandProto instead, which
Review Comment:
Again let's make the deprecation message the first thing in the comment.
##########
hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto:
##########
@@ -498,13 +515,26 @@ message ProcessCRLCommandProto {
/**
* This command asks the DataNode to finalize a new layout version.
+ *
+ * Deprecated, please use FinalizeNewDatanodeVersionCommandProto instead, which
+ * carries a DatanodeVersionProto instead of the deprecated LayoutVersionProto.
*/
message FinalizeNewLayoutVersionCommandProto {
required bool finalizeNewLayoutVersion = 1 [default = false];
required LayoutVersionProto dataNodeLayoutVersion = 2;
required int64 cmdId = 3;
}
+/**
+ * This command asks the DataNode to finalize a new datanode version.
+ * Replaces FinalizeNewLayoutVersionCommandProto.
+ */
+message FinalizeNewDatanodeVersionCommandProto {
+ required bool finalizeNewDatanodeVersion = 1 [default = false];
+ optional DatanodeVersionProto datanodeVersion = 2;
Review 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.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeHeartbeatDispatcher.java:
##########
@@ -97,13 +97,13 @@ public List<SCMCommand<?>>
dispatch(SCMHeartbeatRequestProto heartbeat) {
} else {
- LayoutVersionProto versionInfo = null;
- if (!heartbeat.hasDataNodeLayoutVersion()) {
+ DatanodeVersionProto versionInfo = null;
+ if (!heartbeat.hasDatanodeVersion()) {
Review 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.
##########
hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto:
##########
@@ -77,12 +77,25 @@ enum Status {
/**
* DataNode will advertise its current layout version through
* heartbeat as well as registration mechanism.
+ *
+ * Deprecated, please use DatanodeVersionProto instead. The field names
+ * metadataLayoutVersion and softwareLayoutVersion have been replaced by
+ * apparentVersion and softwareVersion respectively.
Review Comment:
Let's make the deprecation message the first thing in the comment.
##########
hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto:
##########
@@ -77,12 +77,25 @@ enum Status {
/**
* DataNode will advertise its current layout version through
* heartbeat as well as registration mechanism.
+ *
+ * Deprecated, please use DatanodeVersionProto instead. The field names
+ * metadataLayoutVersion and softwareLayoutVersion have been replaced by
+ * apparentVersion and softwareVersion respectively.
*/
message LayoutVersionProto {
required uint32 metadataLayoutVersion = 1;
required uint32 softwareLayoutVersion = 2;
}
+/**
+ * Replaces LayoutVersionProto, using the versioning terminology of
+ * ComponentVersionManager: apparentVersion and softwareVersion.
Review Comment:
```suggestion
* Used by Datanodes to report their versions to SCM.
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeHeartbeatDispatcher.java:
##########
@@ -251,13 +251,13 @@ public Map<SCMCommandProto.Type, Integer>
getCommandsToBeSent() {
}
/**
- * Layout report event payload with origin.
+ * Datanode version report event payload with origin.
*/
- public static class LayoutReportFromDatanode
- extends ReportFromDatanode<LayoutVersionProto> {
+ public static class VersionReportFromDatanode
Review Comment:
Looks like this is unused. I'm not sure what its original purpose was but we
can remove it.
##########
hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto:
##########
@@ -498,13 +515,26 @@ message ProcessCRLCommandProto {
/**
* This command asks the DataNode to finalize a new layout version.
+ *
+ * Deprecated, please use FinalizeNewDatanodeVersionCommandProto instead, which
+ * carries a DatanodeVersionProto instead of the deprecated LayoutVersionProto.
*/
message FinalizeNewLayoutVersionCommandProto {
required bool finalizeNewLayoutVersion = 1 [default = false];
required LayoutVersionProto dataNodeLayoutVersion = 2;
required int64 cmdId = 3;
}
+/**
+ * This command asks the DataNode to finalize a new datanode version.
+ * Replaces FinalizeNewLayoutVersionCommandProto.
Review Comment:
IMO better to have the deprecated command point forward to the new one than
the new one point back to the deprecated one.
```suggestion
* This command asks the Datanode to finalize a new datanode version.
```
--
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]
