xkrogen commented on code in PR #4201:
URL: https://github.com/apache/hadoop/pull/4201#discussion_r923907353
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSHAAdmin.java:
##########
@@ -262,8 +262,13 @@ private int transitionToObserver(final CommandLine cmd)
if (!checkManualStateManagementOK(target)) {
return -1;
}
- HAServiceProtocol proto = target.getProxy(getConf(), 0);
- HAServiceProtocolHelper.transitionToObserver(proto, createReqInfo());
+ try {
+ HAServiceProtocol proto = target.getProxy(getConf(), 0);
+ HAServiceProtocolHelper.transitionToObserver(proto, createReqInfo());
+ } catch (IOException e) {
+ errOut.println("TransitionToObserver failed! " + e.getMessage());
Review Comment:
minor nit: `TransitionToObserver` -> `transitionToObserver` for consistency
with other prints e.g. L254
Also `getMessage()` -> `getLocalizedMessage()`
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdminMiniCluster.java:
##########
@@ -161,7 +164,28 @@ public void testObserverIllegalTransition() throws
Exception {
assertEquals(-1, runTool("-transitionToActive", "nn1"));
assertFalse(nnode1.isActiveState());
}
-
+
+ /**
+ * Tests that a Namenode in safe mode should not be transfer to observer.
+ */
+ @Test
+ public void testObserverTransitionInSafeMode() throws Exception {
+ NameNodeAdapter.enterSafeMode(cluster.getNameNode(0), false);
+ DFSHAAdmin tool = new DFSHAAdmin();
+ tool.setConf(conf);
+ System.setIn(new ByteArrayInputStream("yes\n".getBytes()));
+ int result = tool.run(
+ new String[]{"-transitionToObserver", "-forcemanual", "nn1"});
Review Comment:
minor nit: indentation should be 4 spaces
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestHASafeMode.java:
##########
@@ -977,4 +977,31 @@ public void testTransitionToActiveWhenSafeMode() throws
Exception {
() -> miniCluster.transitionToActive(0));
}
}
+
+ /**
+ * Test transition to observer when namenode in safemode.
+ *
+ * @throws IOException
Review Comment:
this is inaccurate, it throws `Exception`
but this annotation isn't really necessary IMO? it's just a test
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestHASafeMode.java:
##########
@@ -977,4 +977,31 @@ public void testTransitionToActiveWhenSafeMode() throws
Exception {
() -> miniCluster.transitionToActive(0));
}
}
+
+ /**
+ * Test transition to observer when namenode in safemode.
+ *
+ * @throws IOException
+ */
+ @Test
+ public void testTransitionToObserverWhenSafeMode() throws Exception {
+ Configuration config = new Configuration();
+ config.setBoolean(DFS_HA_NN_NOT_BECOME_ACTIVE_IN_SAFEMODE, true);
+ try (MiniDFSCluster miniCluster = new MiniDFSCluster.Builder(config,
+ new File(GenericTestUtils.getRandomizedTempPath()))
+ .nnTopology(MiniDFSNNTopology.simpleHATopology())
+ .numDataNodes(1)
+ .build()) {
+ miniCluster.waitActive();
+ miniCluster.transitionToStandby(0);
+ miniCluster.transitionToStandby(1);
+ NameNode namenode0 = miniCluster.getNameNode(0);
+ NameNode namenode1 = miniCluster.getNameNode(1);
+ NameNodeAdapter.enterSafeMode(namenode0, false);
+ NameNodeAdapter.enterSafeMode(namenode1, false);
+ LambdaTestUtils.intercept(ServiceFailedException.class,
+ "NameNode still not leave safemode",
+ () -> miniCluster.transitionToObserver(0));
Review Comment:
minor nit: indentation should be 4 spaces
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdminMiniCluster.java:
##########
@@ -161,7 +164,28 @@ public void testObserverIllegalTransition() throws
Exception {
assertEquals(-1, runTool("-transitionToActive", "nn1"));
assertFalse(nnode1.isActiveState());
}
-
+
+ /**
+ * Tests that a Namenode in safe mode should not be transfer to observer.
+ */
+ @Test
+ public void testObserverTransitionInSafeMode() throws Exception {
+ NameNodeAdapter.enterSafeMode(cluster.getNameNode(0), false);
+ DFSHAAdmin tool = new DFSHAAdmin();
+ tool.setConf(conf);
+ System.setIn(new ByteArrayInputStream("yes\n".getBytes()));
+ int result = tool.run(
+ new String[]{"-transitionToObserver", "-forcemanual", "nn1"});
+ assertEquals("State transition returned: " + result, -1, result);
+
+ NameNodeAdapter.leaveSafeMode(cluster.getNameNode(0));
+ System.setIn(new ByteArrayInputStream("yes\n".getBytes()));
+ int result1 = tool.run(
+ new String[]{"-transitionToObserver", "-forcemanual", "nn1"});
Review Comment:
minor nit: indentation should be 4 spaces
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSHAAdmin.java:
##########
@@ -262,8 +262,13 @@ private int transitionToObserver(final CommandLine cmd)
if (!checkManualStateManagementOK(target)) {
return -1;
}
- HAServiceProtocol proto = target.getProxy(getConf(), 0);
- HAServiceProtocolHelper.transitionToObserver(proto, createReqInfo());
+ try {
+ HAServiceProtocol proto = target.getProxy(getConf(), 0);
+ HAServiceProtocolHelper.transitionToObserver(proto, createReqInfo());
+ } catch (IOException e) {
Review Comment:
Maybe only catch `ServiceFailedException` here since we only print the
message. If an arbitrary `IOException` occurs, we probably want the additional
context provided by the full stack trace.
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestHASafeMode.java:
##########
@@ -977,4 +977,31 @@ public void testTransitionToActiveWhenSafeMode() throws
Exception {
() -> miniCluster.transitionToActive(0));
}
}
+
+ /**
+ * Test transition to observer when namenode in safemode.
+ *
+ * @throws IOException
+ */
+ @Test
+ public void testTransitionToObserverWhenSafeMode() throws Exception {
+ Configuration config = new Configuration();
+ config.setBoolean(DFS_HA_NN_NOT_BECOME_ACTIVE_IN_SAFEMODE, true);
+ try (MiniDFSCluster miniCluster = new MiniDFSCluster.Builder(config,
+ new File(GenericTestUtils.getRandomizedTempPath()))
+ .nnTopology(MiniDFSNNTopology.simpleHATopology())
+ .numDataNodes(1)
+ .build()) {
Review Comment:
minor nit: indentation should be 4 spaces
--
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]
