xkrogen commented on code in PR #4201:
URL: https://github.com/apache/hadoop/pull/4201#discussion_r863153584
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java:
##########
@@ -1899,6 +1899,10 @@ synchronized void transitionToStandby() throws
IOException {
synchronized void transitionToObserver() throws IOException {
String operationName = "transitionToObserver";
namesystem.checkSuperuserPrivilege(operationName);
+ if (namesystem.isInSafeMode()) {
Review Comment:
I think we can guard this by `notBecomeActiveInSafemode`. Though the config
claims to be about "active" status, the logic in `monitorHealth` just generally
considers a standby NN as unhealthy if it's in safemode, and I think the intent
here is the same as with that config.
Also: `namesystem.isInSafeMode()` -> `isInSafeMode()` (`NameNode` redefines
this method)
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSZKFailoverController.java:
##########
@@ -301,6 +304,40 @@ public void testManualFailoverWithDFSHAAdmin() throws
Exception {
waitForHAState(1, HAServiceState.STANDBY);
}
+ /**
+ * Tests that a Namenode in safe mode should not be transfer to observer
state.
+ */
+ @Test
+ public void testManualFailoverWithDFSHAAdminInSafemode() throws Exception {
+ startCluster();
+ NamenodeProtocols nn1 = cluster.getNameNode(1).getRpcServer();
+
+ // Enter safe mode.
+ nn1.setSafeMode(HdfsConstants.SafeModeAction.SAFEMODE_ENTER, false);
+ // Test NameNodeRpcServer.
+ LambdaTestUtils.intercept(SafeModeException.class,
+ "Cannot transition to observer. Name node is in safe mode",
+ () -> nn1.transitionToObserver(
+ new StateChangeRequestInfo(RequestSource.REQUEST_BY_USER_FORCED)));
+
+ // Test DFSHAAdmin.
+ DFSHAAdmin tool = new DFSHAAdmin();
+ tool.setConf(conf);
+ System.setIn(new ByteArrayInputStream("yes\n".getBytes()));
+ int result = tool.run(
+ new String[]{"-transitionToObserver", "-forcemanual", "nn2"});
+ assertEquals("State transition returned: " + result, -1, result);
Review Comment:
This should be in `TestDFSHAAdminMiniCluster`
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSZKFailoverController.java:
##########
@@ -301,6 +304,40 @@ public void testManualFailoverWithDFSHAAdmin() throws
Exception {
waitForHAState(1, HAServiceState.STANDBY);
}
+ /**
+ * Tests that a Namenode in safe mode should not be transfer to observer
state.
+ */
+ @Test
+ public void testManualFailoverWithDFSHAAdminInSafemode() throws Exception {
+ startCluster();
+ NamenodeProtocols nn1 = cluster.getNameNode(1).getRpcServer();
+
+ // Enter safe mode.
+ nn1.setSafeMode(HdfsConstants.SafeModeAction.SAFEMODE_ENTER, false);
+ // Test NameNodeRpcServer.
+ LambdaTestUtils.intercept(SafeModeException.class,
+ "Cannot transition to observer. Name node is in safe mode",
+ () -> nn1.transitionToObserver(
+ new StateChangeRequestInfo(RequestSource.REQUEST_BY_USER_FORCED)));
Review Comment:
This should probably be in `TestHASafeMode`, where we already have
`testTransitionToActiveWhenSafeMode`
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java:
##########
@@ -1899,6 +1899,10 @@ synchronized void transitionToStandby() throws
IOException {
synchronized void transitionToObserver() throws IOException {
String operationName = "transitionToObserver";
namesystem.checkSuperuserPrivilege(operationName);
+ if (namesystem.isInSafeMode()) {
+ throw namesystem.newSafemodeException("Cannot transition to " +
+ OBSERVER_STATE);
Review Comment:
Consolidate this logic with the exception from `transitionToActive`:
```java
if (notBecomeActiveInSafemode && isInSafeMode()) {
throw new ServiceFailedException(getRole() + " still not leave
safemode");
}
```
I don't think we need to make `newSafemodeException` public, seems fine to
just throw a new exception here?
--
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]
