errose28 commented on code in PR #10570:
URL: https://github.com/apache/ozone/pull/10570#discussion_r3606578066
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestReplicateContainerCommandHandler.java:
##########
@@ -71,20 +84,24 @@ public void testMetrics() {
DatanodeDetails target = MockDatanodeDetails.randomDatanodeDetails();
ReplicateContainerCommand command =
- ReplicateContainerCommand.toTarget(1, target);
+ ReplicateContainerCommand.toTarget(1, target,
HDDSVersion.SOFTWARE_VERSION);
Review Comment:
Maybe we should either restore the `ReplicateContainerCommand#forTest`
factory or add a something similar to `HddsTestUtils` so we don't need to
manually specify the latest version in the common case.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/ComponentVersion.java:
##########
@@ -80,4 +80,26 @@ default boolean isSupportedBy(ComponentVersion other) {
default Optional<? extends UpgradeAction> action() {
return Optional.empty();
}
+
+ /**
+ * Returns the version with the lowest feature set among the given versions,
+ * so callers can pick a version that is mutually supported by all of them.
+ * Comparison is done through {@link #isSupportedBy}, which respects the
+ * negative/unknown-future-version convention, rather than comparing the
+ * opaque {@link #serialize()} values directly.
+ *
+ * @throws IllegalArgumentException if no versions are provided.
+ */
+ static ComponentVersion min(ComponentVersion... versions) {
Review Comment:
We should add a test for this, including the case where UNKNOWN_VERSION is
passed, one element is passed, no elements passed, etc.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -527,10 +527,15 @@ public void sendThrottledReplicationCommand(ContainerInfo
containerInfo,
DatanodeDetails source = selectAndOptionallyExcludeDatanode(
1, sourceWithCmds);
- ReplicateContainerCommand cmd =
- ReplicateContainerCommand.toTarget(containerID, target);
- cmd.setReplicaIndex(replicaIndex);
- sendDatanodeCommand(cmd, containerInfo, source);
+ try {
+ ReplicateContainerCommand cmd = ReplicateContainerCommand.toTarget(
+ containerID, target,
+ nodeManager.getLowestApparentVersion(source, target));
+ cmd.setReplicaIndex(replicaIndex);
+ sendDatanodeCommand(cmd, containerInfo, source);
+ } catch (NodeNotFoundException e) {
+ throw new IllegalArgumentException("Datanode not found in NodeManager.
Should not happen", e);
Review Comment:
We should include the datanode details in the log message like
[this](https://github.com/apache/ozone/blob/14e2d3187f78670d990d94db8720a5fb94d830d2/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java#L498).
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestReplicationManager.java:
##########
@@ -1739,4 +1750,68 @@ private void mockReplicationCommandCounts(
});
}
+ /**
+ * Captures the ReplicateContainerCommand SCM sends to a datanode.
+ */
+ private ReplicateContainerCommand captureSentReplicateCommand() {
+ ArgumentCaptor<SCMCommand<?>> command =
+ ArgumentCaptor.forClass(SCMCommand.class);
+ verify(nodeManager).addDatanodeCommand(any(DatanodeID.class),
+ command.capture());
+ return (ReplicateContainerCommand) command.getValue();
+ }
+
+ private DatanodeInfo mockDatanodeWithApparentVersion(
+ DatanodeDetails dn, HDDSVersion version) {
+ DatanodeInfo info = mock(DatanodeInfo.class);
+ when(info.getLastKnownApparentVersion()).thenReturn(version);
+ when(nodeManager.getNode(dn.getID())).thenReturn(info);
+ return info;
+ }
+
+ @Test
+ public void testApparentVersionIsLowestOfSourceAndTarget()
+ throws NotLeaderException, NodeNotFoundException {
+ ContainerInfo containerInfo =
+ ReplicationTestUtil.createContainerInfo(repConfig, 1,
+ HddsProtos.LifeCycleState.CLOSED, 10, 20);
+ DatanodeDetails target = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails source = MockDatanodeDetails.randomDatanodeDetails();
+
+ // Source is newer than target, so the lowest common version is the
+ // target's version, not the peer the command is sent to.
+ mockDatanodeWithApparentVersion(source, HDDSVersion.STREAM_BLOCK_SUPPORT);
+ mockDatanodeWithApparentVersion(target,
+ HDDSVersion.SEPARATE_RATIS_PORTS_AVAILABLE);
+ when(nodeManager.getLowestApparentVersion(source, target))
+ .thenCallRealMethod();
+
+ replicationManager.sendLowPriorityReplicateContainerCommand(containerInfo,
+ 1, source, target, clock.millis() + rmConf.getEventTimeout());
Review Comment:
We should test the matrix of source and target newer against both
`sendThrottledReplicationCommand` and
`sendLowPriorityReplicateContainerCommand`. This could be combined into one
test method though.
--
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]
