simbadzina commented on code in PR #6018:
URL: https://github.com/apache/hadoop/pull/6018#discussion_r1319140765
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestBootstrapStandby.java:
##########
@@ -172,6 +185,104 @@ public void testDownloadingLaterCheckpoint() throws
Exception {
restartNameNodesFromIndex(1);
}
+ /**
+ * Test for downloading a checkpoint while the cluster is in rolling upgrade.
+ */
+ @Test
+ public void testRollingUpgradeBootstrapStandby() throws Exception {
+ removeStandbyNameDirs();
+
+ int futureVersion = NameNodeLayoutVersion.CURRENT_LAYOUT_VERSION - 1;
+
+ DistributedFileSystem fs = cluster.getFileSystem(0);
+ fs.setSafeMode(SafeModeAction.SAFEMODE_ENTER);
+ fs.saveNamespace();
+ fs.setSafeMode(SafeModeAction.SAFEMODE_LEAVE);
Review Comment:
`setSafeMode(HdfsConstants.SafeModeAction action)` is deprecated. There is a
new method `setSafeMode(SafeModeAction)`
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestBootstrapStandby.java:
##########
@@ -172,6 +185,104 @@ public void testDownloadingLaterCheckpoint() throws
Exception {
restartNameNodesFromIndex(1);
}
+ /**
+ * Test for downloading a checkpoint while the cluster is in rolling upgrade.
+ */
+ @Test
+ public void testRollingUpgradeBootstrapStandby() throws Exception {
Review Comment:
Is it possible to have a unit test that reproduces the error you shared in
the PR description, and then the error is resolved with your new code? Besides
an error code and expecting an expense, this test passes with the old code:
https://github.com/simbadzina/hadoop/pull/2/files
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/BootstrapStandby.java:
##########
@@ -405,8 +423,14 @@ private boolean checkLogsAvailableForRead(FSImage image,
long imageTxId,
}
}
- private boolean checkLayoutVersion(NamespaceInfo nsInfo) throws IOException {
- return (nsInfo.getLayoutVersion() ==
HdfsServerConstants.NAMENODE_LAYOUT_VERSION);
+ private boolean checkLayoutVersion(NamespaceInfo nsInfo, boolean
isRollingUpgrade) {
+ if (isRollingUpgrade) {
+ // During a rolling upgrade the service layout versions may be different,
+ // but we should check that the layout version being sent is compatible
+ return nsInfo.getLayoutVersion() <=
+ HdfsServerConstants.MINIMUM_COMPATIBLE_NAMENODE_LAYOUT_VERSION;
Review Comment:
Shouldn't the comparison here be `>=`, to validate that the layoutVersion is
at least the minimum compatible one.
--
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]
