errose28 commented on code in PR #6155:
URL: https://github.com/apache/ozone/pull/6155#discussion_r1486675502
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestBlockDataStreamOutput.java:
##########
@@ -70,6 +74,7 @@ public class TestBlockDataStreamOutput {
private static String volumeName;
private static String bucketName;
private static String keyString;
+ private static final int DN_CURRENT_VERSION =
DatanodeVersion.SEPARATE_RATIS_PORTS_AVAILABLE.toProtoValue();
Review Comment:
Should we use `DatanodeVersion#CURRENT_VERSION` here instead, or is this
test trying to check that older datanodes return the correct version? I think
the later is a good check, but we should probably not configure this old
version in all the tests, as it may break things in the future. We should
probably also change the variable name to `DN_OLD_VERSION` or something like
that.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneCluster.java:
##########
@@ -321,15 +322,20 @@ abstract class Builder {
protected Optional<Integer> hbProcessorInterval = Optional.empty();
protected String scmId = UUID.randomUUID().toString();
protected String omId = UUID.randomUUID().toString();
-
+
protected Optional<String> datanodeReservedSpace = Optional.empty();
protected boolean includeRecon = false;
-
protected Optional<Integer> omLayoutVersion = Optional.empty();
protected Optional<Integer> scmLayoutVersion = Optional.empty();
protected Optional<Integer> dnLayoutVersion = Optional.empty();
+ protected int dnInitialVersion =
DatanodeVersion.FUTURE_VERSION.toProtoValue();
+ protected int dnCurrentVersion =
DatanodeVersion.FUTURE_VERSION.toProtoValue();
Review Comment:
Shouldn't this default to current version instead of future version?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java:
##########
@@ -396,6 +401,15 @@ private void waitForHddsDatanodeToStop(DatanodeDetails dn)
}, 1000, waitForClusterToBeReadyTimeout);
}
+ private static void overrideDatanodeVersions(int dnInitialVersion, int
dnCurrentVersion) {
+ if (dnInitialVersion != DatanodeVersion.FUTURE_VERSION.toProtoValue()) {
+
mockDNStatic.when(HddsDatanodeService::getDefaultInitialVersion).thenReturn(dnInitialVersion);
+ }
+ if (dnCurrentVersion != DatanodeVersion.FUTURE_VERSION.toProtoValue()) {
+
mockDNStatic.when(HddsDatanodeService::getDefaultCurrentVersion).thenReturn(dnCurrentVersion);
Review Comment:
In this case the "not future version" check makes sense, because the
datanode will never return FUTURE_VERSION directly to the client. Can we add a
comment here explaining this since it may not be intuitive for someone looking
at this for the first time?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java:
##########
@@ -425,14 +424,15 @@ private DatanodeDetails initializeDatanodeDetails()
Preconditions.checkNotNull(idFilePath);
File idFile = new File(idFilePath);
if (idFile.exists()) {
- return ContainerUtils.readDatanodeDetailsFrom(idFile);
+ DatanodeDetails details = ContainerUtils.readDatanodeDetailsFrom(idFile);
+ details.setCurrentVersion(DatanodeVersion.CURRENT_VERSION);
Review Comment:
This should already be set in `ContainerUtils#readDatanodeDetailsFrom`
[here](https://github.com/apache/ozone/blob/54a5a3c59ce450f81d71d8412fcebbd334d2a59e/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/DatanodeIdYaml.java#L116-L117)
--
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]
