[jira] [Work logged] (HDFS-16547) [SBN read] Namenode in safe mode should not be transfered to observer state

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/HDFS-16547?focusedWorklogId=765117&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-765117 ]

ASF GitHub Bot logged work on HDFS-16547:
-----------------------------------------

                Author: ASF GitHub Bot
            Created on: 02/May/22 20:16
            Start Date: 02/May/22 20:16
    Worklog Time Spent: 10m 
      Work Description: 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?





Issue Time Tracking
-------------------

    Worklog Id:     (was: 765117)
    Time Spent: 50m  (was: 40m)

> [SBN read] Namenode in safe mode should not be transfered to observer state
> ---------------------------------------------------------------------------
>
>                 Key: HDFS-16547
>                 URL: https://issues.apache.org/jira/browse/HDFS-16547
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>            Reporter: Tao Li
>            Assignee: Tao Li
>            Priority: Major
>              Labels: pull-request-available
>          Time Spent: 50m
>  Remaining Estimate: 0h
>
> Currently, when a Namenode is in safemode(under starting or enter safemode manually), we can transfer this Namenode to Observer by command. This Observer node may receive many requests and then throw a SafemodeException, this causes unnecessary failover on the client.
> So Namenode in safe mode should not be transfer to observer state.



--
This message was sent by Atlassian Jira
(v8.20.7#820007)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org