errose28 commented on code in PR #10570:
URL: https://github.com/apache/ozone/pull/10570#discussion_r3547228232
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeManager.java:
##########
@@ -197,6 +198,31 @@ default DatanodeFinalizationCounts
getDatanodeFinalizationCounts() {
return new DatanodeFinalizationCounts(finalizedNodes, totalHealthyNodes);
}
+ /**
+ * Returns the lowest apparent (finalized) version among the given datanodes,
+ * so every node involved in an operation uses the same, mutually-supported
+ * version.
+ *
+ * @throws NodeNotFoundException if SCM has no record of one of the nodes;
+ * callers must not proceed with an operation involving a node SCM does
+ * not know about.
+ */
+ default HDDSVersion getLowestApparentVersion(DatanodeDetails... nodes)
+ throws NodeNotFoundException {
+ HDDSVersion lowest = HDDSVersion.SOFTWARE_VERSION;
+ for (DatanodeDetails dn : nodes) {
+ DatanodeInfo info = getDatanodeInfo(dn);
+ if (info == null) {
+ throw new NodeNotFoundException(dn.getID());
+ }
+ HDDSVersion version = info.getApparentHddsVersion();
+ if (version.serialize() < lowest.serialize()) {
Review Comment:
Ideally nothing outside the version class would be comparing serialized
values. Maybe we add a static `ComponentVersion.min(v1, v2)` method to handle
this.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeManager.java:
##########
@@ -197,6 +198,31 @@ default DatanodeFinalizationCounts
getDatanodeFinalizationCounts() {
return new DatanodeFinalizationCounts(finalizedNodes, totalHealthyNodes);
}
+ /**
+ * Returns the lowest apparent (finalized) version among the given datanodes,
Review Comment:
```suggestion
* Returns the lowest apparent version among the given datanodes,
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeInfo.java:
##########
@@ -159,6 +160,33 @@ public ComponentVersion getLastKnownApparentVersion() {
}
}
+ /**
+ * Returns the apparent version last reported by this datanode as
+ * an {@link HDDSVersion}. SCM uses this as the version datanodes should
follow
+ * so that every node involved in an operation agrees on a mutually-supported
+ * version.
+ *
+ * @throws IllegalStateException if the datanode reported a version newer
than
+ * this SCM recognizes. SCM must be upgraded before datanodes, so a newer
+ * datanode is illegal and should have been fenced out of the cluster.
+ */
+ public HDDSVersion getApparentHddsVersion() {
+ ComponentVersion apparentVersion = getLastKnownApparentVersion();
+ if (apparentVersion == null) {
+ // Datanodes are expected to report their version on every heartbeat.
+ // Warn rather than silently defaulting forever if reporting is broken.
+ LOG.warn("Datanode {} has no reported apparent version; falling back " +
+ "to {}.", this, HDDSVersion.DEFAULT_VERSION);
+ return HDDSVersion.DEFAULT_VERSION;
+ }
+ if (apparentVersion == HDDSVersion.UNKNOWN_VERSION) {
+ throw new IllegalStateException("Datanode " + this + " reported an " +
+ "apparent version newer than SCM recognizes. SCM must be upgraded " +
+ "before datanodes.");
+ }
+ return HDDSVersion.deserialize(apparentVersion.serialize());
Review Comment:
The version returned by the datanodes may be an HDDSVersion or an
HDDSLayoutFeature. This in-place serde is just erasing the type. Really this
whole method should be deleted. We should add
`ComponentVersion.min(ComponentVersion...)` and pass it
`getLastKnownApparentVersion` for all Datanodes we are considering. If you
check the constructor of this class you'll see that we already assume the
version info is always present, otherwise we'd get an NPE. That part could
possibly be improved, even though DNs are expected to send version info on
every heartbeat.
--
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]
