HBase
  1. HBase
  2. HBASE-5916

RS restart just before master intialization we make the cluster non operative

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.92.1, 0.94.0
    • Fix Version/s: 0.92.2, 0.94.1, 0.95.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Consider a case where my master is getting restarted. RS that was alive when the master restart started, gets restarted before the master initializes the ServerShutDownHandler.

      serverShutdownHandlerEnabled = true;
      

      In this case when the RS tries to register with the master, the master will try to expire the server but the server cannot be expired as still the serverShutdownHandler is not enabled.

      This case may happen when i have only one RS gets restarted or all the RS gets restarted at the same time.(before assignRootandMeta).

      LOG.info(message);
            if (existingServer.getStartcode() < serverName.getStartcode()) {
              LOG.info("Triggering server recovery; existingServer " +
                existingServer + " looks stale, new server:" + serverName);
              expireServer(existingServer);
            }
      

      If another RS is brought up then the cluster comes back to normalcy.

      May be a very corner case.

      1. HBASE-5916_92.patch
        11 kB
        rajeshbabu
      2. HBASE-5916_trunk_v9.patch
        10 kB
        ramkrishna.s.vasudevan
      3. HBASE-5916_94.patch
        10 kB
        rajeshbabu
      4. HBASE-5916_trunk_v8.patch
        10 kB
        rajeshbabu
      5. HBASE-5916v8.patch
        5 kB
        chunhui shen
      6. HBASE-5916_trunk_v7.patch
        10 kB
        rajeshbabu
      7. HBASE-5916_trunk_v6.patch
        10 kB
        rajeshbabu
      8. HBASE-5916_trunk_v5.patch
        8 kB
        rajeshbabu
      9. HBASE-5916_trunk_4.patch
        7 kB
        ramkrishna.s.vasudevan
      10. HBASE-5916_trunk_3.patch
        12 kB
        ramkrishna.s.vasudevan
      11. HBASE-5916_trunk_2.patch
        11 kB
        ramkrishna.s.vasudevan
      12. HBASE-5916_trunk_1.patch
        11 kB
        ramkrishna.s.vasudevan
      13. HBASE-5916_trunk_1.patch
        11 kB
        ramkrishna.s.vasudevan
      14. HBASE-5916_trunk.patch
        10 kB
        ramkrishna.s.vasudevan

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in HBase-0.92-security #109 (See https://builds.apache.org/job/HBase-0.92-security/109/)
          HBASE-5916 RS restart just before master intialization we make the cluster non operative (Rajesh) (Revision 1343824)

          Result = SUCCESS
          ramkrishna :
          Files :

          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
          • /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Show
          Hudson added a comment - Integrated in HBase-0.92-security #109 (See https://builds.apache.org/job/HBase-0.92-security/109/ ) HBASE-5916 RS restart just before master intialization we make the cluster non operative (Rajesh) (Revision 1343824) Result = SUCCESS ramkrishna : Files : /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94-security #33 (See https://builds.apache.org/job/HBase-0.94-security/33/)
          HBASE-5916 RS restart just before master intialization we make the cluster non operative(RajeshBabu) (Revision 1343326)

          Result = FAILURE
          ramkrishna :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Show
          Hudson added a comment - Integrated in HBase-0.94-security #33 (See https://builds.apache.org/job/HBase-0.94-security/33/ ) HBASE-5916 RS restart just before master intialization we make the cluster non operative(RajeshBabu) (Revision 1343326) Result = FAILURE ramkrishna : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.92 #425 (See https://builds.apache.org/job/HBase-0.92/425/)
          HBASE-5916 RS restart just before master intialization we make the cluster non operative (Rajesh) (Revision 1343824)

          Result = FAILURE
          ramkrishna :
          Files :

          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
          • /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Show
          Hudson added a comment - Integrated in HBase-0.92 #425 (See https://builds.apache.org/job/HBase-0.92/425/ ) HBASE-5916 RS restart just before master intialization we make the cluster non operative (Rajesh) (Revision 1343824) Result = FAILURE ramkrishna : Files : /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Hide
          ramkrishna.s.vasudevan added a comment -

          Committed to 0.92 also.
          Hence resolving it.

          Show
          ramkrishna.s.vasudevan added a comment - Committed to 0.92 also. Hence resolving it.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12530015/HBASE-5916_92.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2028//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12530015/HBASE-5916_92.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2028//console This message is automatically generated.
          Hide
          rajeshbabu added a comment -

          Patch for 92.

          Show
          rajeshbabu added a comment - Patch for 92.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #30 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/30/)
          HBASE-5916 RS restart just before master intialization we make the cluster non operative (Rajesh) (Revision 1343324)

          Result = FAILURE
          ramkrishna :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #30 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/30/ ) HBASE-5916 RS restart just before master intialization we make the cluster non operative (Rajesh) (Revision 1343324) Result = FAILURE ramkrishna : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94 #222 (See https://builds.apache.org/job/HBase-0.94/222/)
          HBASE-5916 RS restart just before master intialization we make the cluster non operative(RajeshBabu) (Revision 1343326)

          Result = FAILURE
          ramkrishna :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Show
          Hudson added a comment - Integrated in HBase-0.94 #222 (See https://builds.apache.org/job/HBase-0.94/222/ ) HBASE-5916 RS restart just before master intialization we make the cluster non operative(RajeshBabu) (Revision 1343326) Result = FAILURE ramkrishna : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2940 (See https://builds.apache.org/job/HBase-TRUNK/2940/)
          HBASE-5916 RS restart just before master intialization we make the cluster non operative (Rajesh) (Revision 1343324)

          Result = SUCCESS
          ramkrishna :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2940 (See https://builds.apache.org/job/HBase-TRUNK/2940/ ) HBASE-5916 RS restart just before master intialization we make the cluster non operative (Rajesh) (Revision 1343324) Result = SUCCESS ramkrishna : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenMasterInitializing.java
          Hide
          ramkrishna.s.vasudevan added a comment - - edited

          Committed to trunk and 0.94.
          Thanks for the review Chunhui, Ted and Stack.
          Thanks to Rajesh for his patches also.

          Show
          ramkrishna.s.vasudevan added a comment - - edited Committed to trunk and 0.94. Thanks for the review Chunhui, Ted and Stack. Thanks to Rajesh for his patches also.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Reattaching for hadoop QA.

          Show
          ramkrishna.s.vasudevan added a comment - Reattaching for hadoop QA.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12529892/HBASE-5916_94.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2014//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12529892/HBASE-5916_94.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2014//console This message is automatically generated.
          Hide
          rajeshbabu added a comment -

          Patch for 94

          Show
          rajeshbabu added a comment - Patch for 94
          Hide
          ramkrishna.s.vasudevan added a comment -

          Pls provide your comments/suggestions on this.
          I will commit this patch tomorrow if there is no objection.

          Show
          ramkrishna.s.vasudevan added a comment - Pls provide your comments/suggestions on this. I will commit this patch tomorrow if there is no objection.
          Hide
          rajeshbabu added a comment -

          Re-based patch as for latest trunk

          Show
          rajeshbabu added a comment - Re-based patch as for latest trunk
          Hide
          ramkrishna.s.vasudevan added a comment -

          I meant RIT is with the original server name only and not yet updated with the new RS. Good on you Chunhui.

          Show
          ramkrishna.s.vasudevan added a comment - I meant RIT is with the original server name only and not yet updated with the new RS. Good on you Chunhui.
          Hide
          chunhui shen added a comment -

          but what if for few regions which are yet to be assigned the RIT is still not updated

          when master startup before initialized, the region will be in the RIT through AssignmentManager#processRegionsInTransition for the above case.

          Show
          chunhui shen added a comment - but what if for few regions which are yet to be assigned the RIT is still not updated when master startup before initialized, the region will be in the RIT through AssignmentManager#processRegionsInTransition for the above case.
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Chunhui

          else if (addressFromAM != null
                          && !addressFromAM.equals(this.serverName)) {
                        LOG.debug("Skip assigning region "
                              + e.getKey().getRegionNameAsString()
                              + " because it has been opened in "
                              + addressFromAM.getServerName());
                        }
          

          Just to clarify this, the assignment will not happen if the address mismatches but what if for few regions which are yet to be assigned the RIT is still not updated. Chunhui, as you said all the cases discussed here are very corner case. Also i really appreciate your help on this making us find out more cases. Thank you very much.
          Let me wait for Stack's comments also.

          Show
          ramkrishna.s.vasudevan added a comment - @Chunhui else if (addressFromAM != null && !addressFromAM.equals( this .serverName)) { LOG.debug( "Skip assigning region " + e.getKey().getRegionNameAsString() + " because it has been opened in " + addressFromAM.getServerName()); } Just to clarify this, the assignment will not happen if the address mismatches but what if for few regions which are yet to be assigned the RIT is still not updated. Chunhui, as you said all the cases discussed here are very corner case. Also i really appreciate your help on this making us find out more cases. Thank you very much. Let me wait for Stack's comments also.
          Hide
          chunhui shen added a comment -

          @ram
          Thanks to write much for the case.
          However, I don't think the above case will happen. Correct me if wrong.

          At the same time as master initialization has already been done and so we are able to carry on assignment with SSH also. This will lead to double assignment

          Why it will lead to double assignment? When we reassign regions in the process of SSH, we would skip regions as the folowing:

          if (processDeadRegion(e.getKey(), e.getValue(),
                        this.services.getAssignmentManager(),
                        this.server.getCatalogTracker())) {
                      ServerName addressFromAM = this.services.getAssignmentManager()
                          .getRegionServerOfRegion(e.getKey());
                      if (rit != null && !rit.isClosing() && !rit.isPendingClose()) {
                        // Skip regions that were in transition unless CLOSING or
                        // PENDING_CLOSE
                        LOG.info("Skip assigning region " + rit.toString());
                      } else if (addressFromAM != null
                          && !addressFromAM.equals(this.serverName)) {
                        LOG.debug("Skip assigning region "
                              + e.getKey().getRegionNameAsString()
                              + " because it has been opened in "
                              + addressFromAM.getServerName());
                        } else {
                          toAssignRegions.add(e.getKey());
                        }
                    }
          

          In RIT?(not closing&&not pendingClose, it won't be these two state in the above case ) -> skip
          Has onlined on other server-> skip

          At last, I think HBASE-5916_trunk_v7.patch is fine, and aggree we check in the patch for the current JIRA.
          Thanks help for my doubt.

          Show
          chunhui shen added a comment - @ram Thanks to write much for the case. However, I don't think the above case will happen. Correct me if wrong. At the same time as master initialization has already been done and so we are able to carry on assignment with SSH also. This will lead to double assignment Why it will lead to double assignment? When we reassign regions in the process of SSH, we would skip regions as the folowing: if (processDeadRegion(e.getKey(), e.getValue(), this .services.getAssignmentManager(), this .server.getCatalogTracker())) { ServerName addressFromAM = this .services.getAssignmentManager() .getRegionServerOfRegion(e.getKey()); if (rit != null && !rit.isClosing() && !rit.isPendingClose()) { // Skip regions that were in transition unless CLOSING or // PENDING_CLOSE LOG.info( "Skip assigning region " + rit.toString()); } else if (addressFromAM != null && !addressFromAM.equals( this .serverName)) { LOG.debug( "Skip assigning region " + e.getKey().getRegionNameAsString() + " because it has been opened in " + addressFromAM.getServerName()); } else { toAssignRegions.add(e.getKey()); } } In RIT?(not closing&&not pendingClose, it won't be these two state in the above case ) -> skip Has onlined on other server-> skip At last, I think HBASE-5916 _trunk_v7.patch is fine, and aggree we check in the patch for the current JIRA. Thanks help for my doubt.
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Chunhui
          I like your idea too. As i said we are planning to raise an improvement activity for master restart and SSH.
          Because even with the above approach i will tell one more scenario which is problematic. Pls note that the scenario can come even without your suggestion also.

          Two region servers are there. Both went down when the flow is in AM.joinCluster(). Now as no RS is there at that time we will not make any assignment. And all will go into RIT mode waiting for timeout monitor. Now SSH is also waiting as the master initialization is not complete(this step is as per your suggestion). Now suppose there are 100 regions all are waiting for getting assigned.
          Now if a new RS comes up as there is a code in TimeoutMonitor

           if (regionState.getStamp() + timeout <= now) {
                     //decide on action upon timeout
                      actOnTimeOut(regionState);
                    } else if (this.allRegionServersOffline && !allRSsOffline) {
                      // if some RSs just came back online, we can start the
                      // the assignment right away
                      actOnTimeOut(regionState);
                    }
          

          It will immediately trigger assignment. At the same time as master initialization has already been done and so we are able to carry on assignment with SSH also. This will lead to double assignment. Actually in defect HBASe-5816 Stack was suggesting to have one common queue where any assignment will be done so that SSH will not interfere with that or viceversa.
          I suggest we can get in the patch that addresses the current JIRa problem and work on a diff JIRA that will help me to address the master restart and SSH area which is troublesome.

          Show
          ramkrishna.s.vasudevan added a comment - @Chunhui I like your idea too. As i said we are planning to raise an improvement activity for master restart and SSH. Because even with the above approach i will tell one more scenario which is problematic. Pls note that the scenario can come even without your suggestion also. Two region servers are there. Both went down when the flow is in AM.joinCluster(). Now as no RS is there at that time we will not make any assignment. And all will go into RIT mode waiting for timeout monitor. Now SSH is also waiting as the master initialization is not complete(this step is as per your suggestion). Now suppose there are 100 regions all are waiting for getting assigned. Now if a new RS comes up as there is a code in TimeoutMonitor if (regionState.getStamp() + timeout <= now) { //decide on action upon timeout actOnTimeOut(regionState); } else if ( this .allRegionServersOffline && !allRSsOffline) { // if some RSs just came back online, we can start the // the assignment right away actOnTimeOut(regionState); } It will immediately trigger assignment. At the same time as master initialization has already been done and so we are able to carry on assignment with SSH also. This will lead to double assignment. Actually in defect HBASe-5816 Stack was suggesting to have one common queue where any assignment will be done so that SSH will not interfere with that or viceversa. I suggest we can get in the patch that addresses the current JIRa problem and work on a diff JIRA that will help me to address the master restart and SSH area which is troublesome.
          Hide
          chunhui shen added a comment -

          We should implement that in a separate issue.

          I think the above suggestion would fix this issue.

          @ram
          Could you give some comments, correct me if anything I didn't consider.

          Show
          chunhui shen added a comment - We should implement that in a separate issue. I think the above suggestion would fix this issue. @ram Could you give some comments, correct me if anything I didn't consider.
          Hide
          Ted Yu added a comment -

          @Chunhui:
          Your suggestion is interesting.
          We should implement that in a separate issue.

          Show
          Ted Yu added a comment - @Chunhui: Your suggestion is interesting. We should implement that in a separate issue.
          Hide
          chunhui shen added a comment -

          @ram
          The above you mentioned is a good case.

          However, I find the current master logic when startup is more and more complicated.
          What about do the following in the process of SSH:

          ...
          if (isCarryingRoot()){}
          if (isCarryingMeta()) {}
           if (isCarryingRoot() || isCarryingMeta()) {}
          int waitedTimeForMasterInitialized = 0;
              while (!server.isStopped() && !services.isInitialized()) {
                try {
                  if (waitedTimeForMasterInitialized == 0) {
                    LOG.info("Master is not initialized, waiting...");
                  }
                  Thread.sleep(100);
                  waitedTimeForMasterInitialized += 100;
                } catch (InterruptedException e) {
                  Thread.currentThread().interrupt();
                  throw new IOException("Interrupted", e);
                }
              }
              if (waitedTimeForMasterInitialized > 0) {
                LOG.info("Recovery time calculation: waiting on master to be initialized took "
                    + waitedTimeForMasterInitialized + "ms");
              }
          
          

          I think we could make SSH wait until master initialized after it assigned META region, thus we could skip considering many troublesome concurrent case .

          Show
          chunhui shen added a comment - @ram The above you mentioned is a good case. However, I find the current master logic when startup is more and more complicated. What about do the following in the process of SSH: ... if (isCarryingRoot()){} if (isCarryingMeta()) {} if (isCarryingRoot() || isCarryingMeta()) {} int waitedTimeForMasterInitialized = 0; while (!server.isStopped() && !services.isInitialized()) { try { if (waitedTimeForMasterInitialized == 0) { LOG.info( "Master is not initialized, waiting..." ); } Thread .sleep(100); waitedTimeForMasterInitialized += 100; } catch (InterruptedException e) { Thread .currentThread().interrupt(); throw new IOException( "Interrupted" , e); } } if (waitedTimeForMasterInitialized > 0) { LOG.info( "Recovery time calculation: waiting on master to be initialized took " + waitedTimeForMasterInitialized + "ms" ); } I think we could make SSH wait until master initialized after it assigned META region, thus we could skip considering many troublesome concurrent case .
          Hide
          ramkrishna.s.vasudevan added a comment -

          First of all thanks for your time in preparing a patch.
          I think if we don't get the new online servers in joincluster there is one problem

               STEP 1: this.serverManager.expireDeadNotExpiredServers();
          
              // Update meta with new HRI if required. i.e migrate all HRI with HTD to
              // HRI with out HTD in meta and update the status in ROOT. This must happen
              // before we assign all user regions or else the assignment will fail.
              // TODO: Remove this when we do 0.94.
            STEP 2:    org.apache.hadoop.hbase.catalog.MetaMigrationRemovingHTD.
                updateMetaWithNewHRI(this);
          
              // Fixup assignment manager status
              status.setStatus("Starting assignment manager");
            STEP 3:    this.assignmentManager.joinCluster(onlineServers);
          

          I will tell you one scenario, may be its too rare but still possible
          I have 3 RS at STEP 1.
          one of them goes down and the SSH processes and tries to assign the regions.
          Before the assignment is done one new RS comes up before STEP 3.
          There is a small chance that the regions from dead RS are assigned to this new RS. Now in step 3 as we have already got the online servers list we may end up in thinking the new RS as an offline server after scanning META. Pls do correct me. Its a corner case.

          Show
          ramkrishna.s.vasudevan added a comment - First of all thanks for your time in preparing a patch. I think if we don't get the new online servers in joincluster there is one problem STEP 1: this .serverManager.expireDeadNotExpiredServers(); // Update meta with new HRI if required. i.e migrate all HRI with HTD to // HRI with out HTD in meta and update the status in ROOT. This must happen // before we assign all user regions or else the assignment will fail. // TODO: Remove this when we do 0.94. STEP 2: org.apache.hadoop.hbase.catalog.MetaMigrationRemovingHTD. updateMetaWithNewHRI( this ); // Fixup assignment manager status status.setStatus( "Starting assignment manager" ); STEP 3: this .assignmentManager.joinCluster(onlineServers); I will tell you one scenario, may be its too rare but still possible I have 3 RS at STEP 1. one of them goes down and the SSH processes and tries to assign the regions. Before the assignment is done one new RS comes up before STEP 3. There is a small chance that the regions from dead RS are assigned to this new RS. Now in step 3 as we have already got the online servers list we may end up in thinking the new RS as an offline server after scanning META. Pls do correct me. Its a corner case.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12529660/HBASE-5916v8.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 34 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1993//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1993//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1993//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12529660/HBASE-5916v8.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 34 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1993//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1993//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1993//console This message is automatically generated.
          Hide
          chunhui shen added a comment -

          I have make a simple patch(v8) with my above mentioned solution

          @ram
          Could you test it with your test case.

          Maybe something wrong, thanks for the reivew.

          Show
          chunhui shen added a comment - I have make a simple patch(v8) with my above mentioned solution @ram Could you test it with your test case. Maybe something wrong, thanks for the reivew.
          Hide
          rajeshbabu added a comment -

          @Zhihong Yu
          Thanks for help.

          In latest patch addressed Zhihong Yu comments.

          Show
          rajeshbabu added a comment - @Zhihong Yu Thanks for help. In latest patch addressed Zhihong Yu comments.
          Hide
          chunhui shen added a comment -

          @ram

          In joinCluster(), as per the existing code if any new server has checked in and the root/meta had got assigned to it in joincluster we may think that it is an dead server because we alerady have passed the online servers.

          If we consider it as a dead server, what error will be caused?
          I think no error. Because, it must be a new regionserver (which is restarted right now), there is no regions carried by it. Of course, we won't assign region to it, but I think it is nothing.

          Correct me if wrong, thanks

          Show
          chunhui shen added a comment - @ram In joinCluster(), as per the existing code if any new server has checked in and the root/meta had got assigned to it in joincluster we may think that it is an dead server because we alerady have passed the online servers. If we consider it as a dead server, what error will be caused? I think no error. Because, it must be a new regionserver (which is restarted right now), there is no regions carried by it. Of course, we won't assign region to it, but I think it is nothing. Correct me if wrong, thanks
          Hide
          Ted Yu added a comment -

          The failure in TestClockSkewDetection was due to NPE.
          The following change makes it pass:

              if ((this.services == null || ((HMaster) this.services).isInitialized())
                  && this.deadservers.cleanPreviousInstance(serverName)) {
          
          +   * To clear any dead server with same host name and port of online server
          

          I think 'any' should be added in front of 'online server'.

          +  public void clearDeadServersWithSameHostNameAndPortOfOnlineServer() {
          

          The above method can be package private, right ?

          +      while ((sn = ServerName.findServerWithSameHostnamePort(this.deadservers, serverName)) != null) {
          

          The above line exceeds 100 chars.

          +      if(actualDeadServers.contains(deadServer.getKey())){
          

          Add spaces after if and before {.

          Show
          Ted Yu added a comment - The failure in TestClockSkewDetection was due to NPE. The following change makes it pass: if (( this .services == null || ((HMaster) this .services).isInitialized()) && this .deadservers.cleanPreviousInstance(serverName)) { + * To clear any dead server with same host name and port of online server I think 'any' should be added in front of 'online server'. + public void clearDeadServersWithSameHostNameAndPortOfOnlineServer() { The above method can be package private, right ? + while ((sn = ServerName.findServerWithSameHostnamePort( this .deadservers, serverName)) != null ) { The above line exceeds 100 chars. + if (actualDeadServers.contains(deadServer.getKey())){ Add spaces after if and before {.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12529160/HBASE-5916_trunk_v6.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 34 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.master.TestClockSkewDetection

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1986//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1986//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1986//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12529160/HBASE-5916_trunk_v6.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 34 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.master.TestClockSkewDetection Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1986//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1986//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1986//console This message is automatically generated.
          Hide
          rajeshbabu added a comment -

          Attached patch. Please review and provide suggestions/comments

          Show
          rajeshbabu added a comment - Attached patch. Please review and provide suggestions/comments
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Chunhui
          The suggestion given above can simply be avoided by taking a the actual online servers list after getting the logFolders. This will ensure that we donot split any new RS that has checked in.

          In joinCluster(), as per the existing code if any new server has checked in and the root/meta had got assigned to it in joincluster we may think that it is an dead server because we alerady have passed the online servers. Hence we are trying to get the actual online list as per the patch.

          The problem that you have mentioned here

          if Regionserver A with startcode 001 is restarted, and then Regionserver A with startcode 002 is in the onlineServers, but Regionserver A with startcode 001 is in the process by SSH, not in the deadServers

          This we are trying to avoid in our current v6 patch, by not remvoing from dead servers, any restarted server that is coming up during master initialization. Later after master initialization we try to clear the dead server which matches with the current online servers with same host name and port.

          There are other problems during SSH and master initialization that may lead to double assignment or concurrent modification exception. These things we will address in a new JIRA.
          Pls review the current patch and provide your suggestions.

          Show
          ramkrishna.s.vasudevan added a comment - @Chunhui The suggestion given above can simply be avoided by taking a the actual online servers list after getting the logFolders. This will ensure that we donot split any new RS that has checked in. In joinCluster(), as per the existing code if any new server has checked in and the root/meta had got assigned to it in joincluster we may think that it is an dead server because we alerady have passed the online servers. Hence we are trying to get the actual online list as per the patch. The problem that you have mentioned here if Regionserver A with startcode 001 is restarted, and then Regionserver A with startcode 002 is in the onlineServers, but Regionserver A with startcode 001 is in the process by SSH, not in the deadServers This we are trying to avoid in our current v6 patch, by not remvoing from dead servers, any restarted server that is coming up during master initialization. Later after master initialization we try to clear the dead server which matches with the current online servers with same host name and port. There are other problems during SSH and master initialization that may lead to double assignment or concurrent modification exception. These things we will address in a new JIRA. Pls review the current patch and provide your suggestions.
          Hide
          Ted Yu added a comment -

          It would help readers understand better if the above code is formulated as a patch.

          Show
          Ted Yu added a comment - It would help readers understand better if the above code is formulated as a patch.
          Hide
          chunhui shen added a comment -

          @ram

          the RS is slow in checking in.

          First, we will do the following:Check zk for regionservers that are up but didn't register.

          Anyway, if the RS is check in when master splitting log, I think we could specify log dirs to split for splitLogAfterStartup.

          We could do it as the following:

          HMaster#finishInitialization{
          ...
          Path logsDirPath = new Path(this.rootdir, HConstants.HREGION_LOGDIR_NAME);
          FileStatus[] logFolders = FSUtils.listStatus(this.fs, logsDirPath, null);
          Set<ServerName> onlineServers = new HashSet<ServerName>(serverManager
                  .getOnlineServers().keySet());
          
          List<ServerName> needSplitServers= new ArrayList<ServerName>();
          for (FileStatus status : logFolders){
          String sn = status.getPath().getName();
          // truncate splitting suffix if present (for ServerName parsing)
          if (sn.endsWith(HLog.SPLITTING_EXT)) {
                    sn = sn.substring(0, sn.length() - HLog.SPLITTING_EXT.length());
          }
          ServerName serverName = ServerName.parseServerName(sn);
          if (!onlineServers.contains(serverName)) {
          needSplitServers.add(serverName)
          }
          }
          ...
          splitLogAfterStartup(mfs,needSplitServers)
          }
          

          Could the above fix the case?
          Correct me if wrong, Thanks

          Show
          chunhui shen added a comment - @ram the RS is slow in checking in. First, we will do the following:Check zk for regionservers that are up but didn't register. Anyway, if the RS is check in when master splitting log, I think we could specify log dirs to split for splitLogAfterStartup. We could do it as the following: HMaster#finishInitialization{ ... Path logsDirPath = new Path( this .rootdir, HConstants.HREGION_LOGDIR_NAME); FileStatus[] logFolders = FSUtils.listStatus( this .fs, logsDirPath, null ); Set<ServerName> onlineServers = new HashSet<ServerName>(serverManager .getOnlineServers().keySet()); List<ServerName> needSplitServers= new ArrayList<ServerName>(); for (FileStatus status : logFolders){ String sn = status.getPath().getName(); // truncate splitting suffix if present ( for ServerName parsing) if (sn.endsWith(HLog.SPLITTING_EXT)) { sn = sn.substring(0, sn.length() - HLog.SPLITTING_EXT.length()); } ServerName serverName = ServerName.parseServerName(sn); if (!onlineServers.contains(serverName)) { needSplitServers.add(serverName) } } ... splitLogAfterStartup(mfs,needSplitServers) } Could the above fix the case? Correct me if wrong, Thanks
          Hide
          ramkrishna.s.vasudevan added a comment -

          Am trying to check all your comments, but coming to this

          if (services.isServerShutdownHandlerEnabled()) {
          +        // master has completed the initialization
          +        throw new PleaseHoldException(message);
          +      }
          

          Anyway as i mentioned there is a chance of HLog file getting deleted. See my comments in https://issues.apache.org/jira/browse/HBASE-5916?focusedCommentId=13267205&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13267205

          That is also one reason why i did not want to go with only that change. Wanted to handle most of the cases. But there are many scenarios here

          Show
          ramkrishna.s.vasudevan added a comment - Am trying to check all your comments, but coming to this if (services.isServerShutdownHandlerEnabled()) { + // master has completed the initialization + throw new PleaseHoldException(message); + } Anyway as i mentioned there is a chance of HLog file getting deleted. See my comments in https://issues.apache.org/jira/browse/HBASE-5916?focusedCommentId=13267205&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13267205 That is also one reason why i did not want to go with only that change. Wanted to handle most of the cases. But there are many scenarios here
          Hide
          chunhui shen added a comment -

          I have a doubt about patchV5

          +    Set<ServerName> actualDeadServers = this.serverManager.getDeadServers();
               for (Map.Entry<ServerName, List<Pair<HRegionInfo, Result>>> deadServer: deadServers.entrySet()) {
          +      // skip regions of dead servers because SSH will process regions during rs expiration. 
          +      // see HBASE-5916
          +      if(actualDeadServers.contains(deadServer.getKey())){
          +        continue;
          +      }
          

          Let's see the ServerManager#getDeadServers()

          public Set<ServerName> getDeadServers() {
              return this.deadservers.clone();
            }
          public synchronized Set<ServerName> clone() {
              Set<ServerName> clone = new HashSet<ServerName>(this.deadServers.size());
              clone.addAll(this.deadServers);
              return clone;
            }
          public boolean cleanPreviousInstance(final ServerName newServerName) {
              ServerName sn =
                ServerName.findServerWithSameHostnamePort(this.deadServers, newServerName);
              if (sn == null) return false;
              return this.deadServers.remove(sn);
            }
          

          if Regionserver A with startcode 001 is restarted, and then Regionserver A with startcode 002 is in the onlineServers, but Regionserver A with startcode 001 is in the process by SSH, not in the deadServers

          So we will multi assign regions carried by the Regionserver A with startcode 001.

          BTW, to fix this issue, why doing the following is not enough?

          -      throw new PleaseHoldException(message);
          +      if (services.isServerShutdownHandlerEnabled()) {
          +        // master has completed the initialization
          +        throw new PleaseHoldException(message);
          +      }

          Correct me if wrong, Thanks

          Show
          chunhui shen added a comment - I have a doubt about patchV5 + Set<ServerName> actualDeadServers = this .serverManager.getDeadServers(); for (Map.Entry<ServerName, List<Pair<HRegionInfo, Result>>> deadServer: deadServers.entrySet()) { + // skip regions of dead servers because SSH will process regions during rs expiration. + // see HBASE-5916 + if (actualDeadServers.contains(deadServer.getKey())){ + continue ; + } Let's see the ServerManager#getDeadServers() public Set<ServerName> getDeadServers() { return this .deadservers.clone(); } public synchronized Set<ServerName> clone() { Set<ServerName> clone = new HashSet<ServerName>( this .deadServers.size()); clone.addAll( this .deadServers); return clone; } public boolean cleanPreviousInstance( final ServerName newServerName) { ServerName sn = ServerName.findServerWithSameHostnamePort( this .deadServers, newServerName); if (sn == null ) return false ; return this .deadServers.remove(sn); } if Regionserver A with startcode 001 is restarted, and then Regionserver A with startcode 002 is in the onlineServers, but Regionserver A with startcode 001 is in the process by SSH, not in the deadServers So we will multi assign regions carried by the Regionserver A with startcode 001. BTW, to fix this issue, why doing the following is not enough? - throw new PleaseHoldException(message); + if (services.isServerShutdownHandlerEnabled()) { + // master has completed the initialization + throw new PleaseHoldException(message); + } Correct me if wrong, Thanks
          Hide
          Ted Yu added a comment -

          +1 on patch v5.

          Minor comment:
          Insert a space after if.

          +      if(actualDeadServers.contains(deadServer.getKey())){
          
          Show
          Ted Yu added a comment - +1 on patch v5. Minor comment: Insert a space after if. + if (actualDeadServers.contains(deadServer.getKey())){
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12527821/HBASE-5916_trunk_v5.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 33 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1910//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1910//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1910//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12527821/HBASE-5916_trunk_v5.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 33 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1910//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1910//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1910//console This message is automatically generated.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Pls check the latest patch. We have tested it on a cluster also. The test case in the first patch will show what is the problem. With this patch i have not attached any new test case.
          Pls take a look at it.

          Show
          ramkrishna.s.vasudevan added a comment - Pls check the latest patch. We have tested it on a cluster also. The test case in the first patch will show what is the problem. With this patch i have not attached any new test case. Pls take a look at it.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12526389/HBASE-5916_trunk_4.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.TestDrainingServer
          org.apache.hadoop.hbase.regionserver.TestRSKilledWhenMasterInitializing

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1836//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1836//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1836//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12526389/HBASE-5916_trunk_4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.TestDrainingServer org.apache.hadoop.hbase.regionserver.TestRSKilledWhenMasterInitializing Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1836//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1836//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1836//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          For MasterFileSystem.java:

          -          if (!onlineServers.contains(serverName)) {
          +          if (!((HMaster) master).getServerManager().getOnlineServers()
          +              .keySet().contains(serverName)) {
          

          The above code is called inside a loop. Please store ((HMaster) master).getServerManager().getOnlineServers() in a variable outside the loop.

          Show
          Ted Yu added a comment - For MasterFileSystem.java: - if (!onlineServers.contains(serverName)) { + if (!((HMaster) master).getServerManager().getOnlineServers() + .keySet().contains(serverName)) { The above code is called inside a loop. Please store ((HMaster) master).getServerManager().getOnlineServers() in a variable outside the loop.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Attached latest patch. Another way but this one would be simpler. Did not write testcase based on the current fix.

          Show
          ramkrishna.s.vasudevan added a comment - Attached latest patch. Another way but this one would be simpler. Did not write testcase based on the current fix.
          Hide
          ramkrishna.s.vasudevan added a comment -

          I have tried out someother way. Will upload the patch tomorrow. But the current testcase in this patch needs to be modified.

          Show
          ramkrishna.s.vasudevan added a comment - I have tried out someother way. Will upload the patch tomorrow. But the current testcase in this patch needs to be modified.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Atleast i can be sure of the RS and master time clocks right?

          Show
          ramkrishna.s.vasudevan added a comment - Atleast i can be sure of the RS and master time clocks right?
          Hide
          ramkrishna.s.vasudevan added a comment -

          if a difference between master and namenode clocks

          I had similar doubts in using time factor. Let me check if something better is available.

          Show
          ramkrishna.s.vasudevan added a comment - if a difference between master and namenode clocks I had similar doubts in using time factor. Let me check if something better is available.
          Hide
          stack added a comment -

          Why make this change?

          -      final Set<ServerName> onlineServers)
          +       Set<ServerName> onlineServers)
          

          Will alreadyOnlineSlowRS be a superset of onlineServers so you only need check it rather than both as in the below?

          +      } else if (!onlineServers.contains(regionLocation)
          +          && !alreadyOnlineSlowRS.contains(regionLocation)) {
          

          I'm not sure master time is what you want. What you want is the filesystem time, the time the namenode is using. I'm not sure but my guess would be that the modtime for files in hdfs would be set by the namenode; if a difference between master and namenode clocks, there could be a hole through which some WALs could slip if we use master time? What you think Ram?

          Otherwise, the patch is great. I love the test.

          Show
          stack added a comment - Why make this change? - final Set<ServerName> onlineServers) + Set<ServerName> onlineServers) Will alreadyOnlineSlowRS be a superset of onlineServers so you only need check it rather than both as in the below? + } else if (!onlineServers.contains(regionLocation) + && !alreadyOnlineSlowRS.contains(regionLocation)) { I'm not sure master time is what you want. What you want is the filesystem time, the time the namenode is using. I'm not sure but my guess would be that the modtime for files in hdfs would be set by the namenode; if a difference between master and namenode clocks, there could be a hole through which some WALs could slip if we use master time? What you think Ram? Otherwise, the patch is great. I love the test.
          Hide
          ramkrishna.s.vasudevan added a comment -

          The test cases are passing this time.
          @Stack
          Your comments on the patch?

          Show
          ramkrishna.s.vasudevan added a comment - The test cases are passing this time. @Stack Your comments on the patch?
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525878/HBASE-5916_trunk_3.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1789//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1789//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1789//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12525878/HBASE-5916_trunk_3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1789//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1789//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1789//console This message is automatically generated.
          Hide
          ramkrishna.s.vasudevan added a comment -

          The patch still applies correctly in Windows thro SVN client. But finally its because there is an empty line in the line number 33.
          Tried out in the apache linux machine.

          Patching file src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java using Plan A...
          Hunk #1 succeeded at 23.
          Hunk #2 succeeded at 37.
          Hunk #3 succeeded at 52.
          Hunk #4 succeeded at 137.
          Hmm...  The next patch looks like a unified diff to me...
          The text leading up to this was:
          

          This time it should run.

          Show
          ramkrishna.s.vasudevan added a comment - The patch still applies correctly in Windows thro SVN client. But finally its because there is an empty line in the line number 33. Tried out in the apache linux machine. Patching file src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java using Plan A... Hunk #1 succeeded at 23. Hunk #2 succeeded at 37. Hunk #3 succeeded at 52. Hunk #4 succeeded at 137. Hmm... The next patch looks like a unified diff to me... The text leading up to this was: This time it should run.
          Hide
          Ted Yu added a comment -

          There is some conflict in TestMasterFailover.java which patch command couldn't resolve:

          ***************
          *** 38,43 ****
            import org.apache.hadoop.hbase.*;
            import org.apache.hadoop.hbase.executor.EventHandler.EventType;
            import org.apache.hadoop.hbase.master.AssignmentManager.RegionState;
            import org.apache.hadoop.hbase.regionserver.HRegion;
            import org.apache.hadoop.hbase.regionserver.HRegionServer;
            import org.apache.hadoop.hbase.util.Bytes;
          --- 39,45 ----
            import org.apache.hadoop.hbase.*;
            import org.apache.hadoop.hbase.executor.EventHandler.EventType;
            import org.apache.hadoop.hbase.master.AssignmentManager.RegionState;
          + import org.apache.hadoop.hbase.master.metrics.MasterMetrics;
            import org.apache.hadoop.hbase.regionserver.HRegion;
            import org.apache.hadoop.hbase.regionserver.HRegionServer;
            import org.apache.hadoop.hbase.util.Bytes;
          
          Show
          Ted Yu added a comment - There is some conflict in TestMasterFailover.java which patch command couldn't resolve: *************** *** 38,43 **** import org.apache.hadoop.hbase.*; import org.apache.hadoop.hbase.executor.EventHandler.EventType; import org.apache.hadoop.hbase.master.AssignmentManager.RegionState; import org.apache.hadoop.hbase.regionserver.HRegion; import org.apache.hadoop.hbase.regionserver.HRegionServer; import org.apache.hadoop.hbase.util.Bytes; --- 39,45 ---- import org.apache.hadoop.hbase.*; import org.apache.hadoop.hbase.executor.EventHandler.EventType; import org.apache.hadoop.hbase.master.AssignmentManager.RegionState; + import org.apache.hadoop.hbase.master.metrics.MasterMetrics; import org.apache.hadoop.hbase.regionserver.HRegion; import org.apache.hadoop.hbase.regionserver.HRegionServer; import org.apache.hadoop.hbase.util.Bytes;
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525870/HBASE-5916_trunk_2.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1788//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12525870/HBASE-5916_trunk_2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1788//console This message is automatically generated.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Reattaching. Patch is getting applied cleanly. May be because of same name the QA did not pick it up.
          Please provide your comments on the latest patch.

          Show
          ramkrishna.s.vasudevan added a comment - Reattaching. Patch is getting applied cleanly. May be because of same name the QA did not pick it up. Please provide your comments on the latest patch.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525729/HBASE-5916_trunk_1.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1779//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12525729/HBASE-5916_trunk_1.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1779//console This message is automatically generated.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Reattaching for hadoopqa.

          Show
          ramkrishna.s.vasudevan added a comment - Reattaching for hadoopqa.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Will make use of hadoopqa to run the testcases, as am at home.

          Show
          ramkrishna.s.vasudevan added a comment - Will make use of hadoopqa to run the testcases, as am at home.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Updated patch.

          Show
          ramkrishna.s.vasudevan added a comment - Updated patch.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Failure in testcase

          java.lang.ClassCastException: org.apache.hadoop.hbase.Server$$EnhancerByMockitoWithCGLIB$$9bdf744 cannot be cast to org.apache.hadoop.hbase.master.HMaster
          	at org.apache.hadoop.hbase.master.AssignmentManager.rebuildUserRegions(AssignmentManager.java:2480)
          	at org.apache.hadoop.hbase.master.AssignmentManager.joinCluster(AssignmentManager.java:359)
          	at org.apache.hadoop.hbase.master.AssignmentManager.joinCluster(AssignmentManager.java:379)
          	at org.apache.hadoop.hbase.master.TestAssignmentManager$2.run(TestAssignmentManager.java:705)
          

          The above exception is due to the patch

          Show
          ramkrishna.s.vasudevan added a comment - Failure in testcase java.lang.ClassCastException: org.apache.hadoop.hbase.Server$$EnhancerByMockitoWithCGLIB$$9bdf744 cannot be cast to org.apache.hadoop.hbase.master.HMaster at org.apache.hadoop.hbase.master.AssignmentManager.rebuildUserRegions(AssignmentManager.java:2480) at org.apache.hadoop.hbase.master.AssignmentManager.joinCluster(AssignmentManager.java:359) at org.apache.hadoop.hbase.master.AssignmentManager.joinCluster(AssignmentManager.java:379) at org.apache.hadoop.hbase.master.TestAssignmentManager$2.run(TestAssignmentManager.java:705) The above exception is due to the patch
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525639/HBASE-5916_trunk.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol
          org.apache.hadoop.hbase.master.TestAssignmentManager

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1766//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1766//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1766//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12525639/HBASE-5916_trunk.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol org.apache.hadoop.hbase.master.TestAssignmentManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1766//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1766//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1766//console This message is automatically generated.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Ooh.. Yes it is HBASE-5916.

          Show
          ramkrishna.s.vasudevan added a comment - Ooh.. Yes it is HBASE-5916 .
          Hide
          Ted Yu added a comment -
          +   * Verifies HBASE-5816. Here before the master splits the HLog if any new RS checks in it should
          

          The JIRA should be HBASE-5916, right ?

          Show
          Ted Yu added a comment - + * Verifies HBASE-5816. Here before the master splits the HLog if any new RS checks in it should The JIRA should be HBASE-5916 , right ?
          Hide
          ramkrishna.s.vasudevan added a comment -

          Just a try, need to do some more testing on it. Uploaded to get some feedback.

          Show
          ramkrishna.s.vasudevan added a comment - Just a try, need to do some more testing on it. Uploaded to get some feedback.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Tried to address this problem using the system time. The test case attached in the patch reproduces the issue of how the HLog gets deleted.
          If the way of using time to avoid this issue seems bad pls don't Hate me. I think atleast the test case will be useful.
          I tried to use the zk but the problem is we will not be sure if that server is about to be expired server or current new server.

          Show
          ramkrishna.s.vasudevan added a comment - Tried to address this problem using the system time. The test case attached in the patch reproduces the issue of how the HLog gets deleted. If the way of using time to avoid this issue seems bad pls don't Hate me. I think atleast the test case will be useful. I tried to use the zk but the problem is we will not be sure if that server is about to be expired server or current new server.
          Hide
          ramkrishna.s.vasudevan added a comment -

          We should not remove PleaseHoldException(message) directly.

                if (services.isServerShutdownHandlerEnabled()) {
                  // master has completed the initialization
                  throw new PleaseHoldException(message);
                }
          

          This solved the actual problem. But the problem due to filenotfoundException should be addressed in a different way.

          Show
          ramkrishna.s.vasudevan added a comment - We should not remove PleaseHoldException(message) directly. if (services.isServerShutdownHandlerEnabled()) { // master has completed the initialization throw new PleaseHoldException(message); } This solved the actual problem. But the problem due to filenotfoundException should be addressed in a different way.
          Hide
          stack added a comment -

          Well, thats useful, right? Its useful in case where a regionserver crashes and a new one comes up fast, before the original regionserver's znode has expired in zk. We shouldn't remove it.

          On startup, you should not get this exception unless you have a condition like that described above where there was a regionserver on same host and port registered previously in the master and then a new regionserver comes in w/ same host and port but with different startcode?

          Show
          stack added a comment - Well, thats useful, right? Its useful in case where a regionserver crashes and a new one comes up fast, before the original regionserver's znode has expired in zk. We shouldn't remove it. On startup, you should not get this exception unless you have a condition like that described above where there was a regionserver on same host and port registered previously in the master and then a new regionserver comes in w/ same host and port but with different startcode?
          Hide
          chunhui shen added a comment -

          We have also encountered this issue.

          What about remove

          throw new PleaseHoldException(message);
          

          in ServerManager#checkAlreadySameHostPort

          Show
          chunhui shen added a comment - We have also encountered this issue. What about remove throw new PleaseHoldException(message); in ServerManager#checkAlreadySameHostPort
          Hide
          stack added a comment -

          So though the server is online i will not be able to use the hLog and i get filenotfoundException.

          Master should not take over hlogs that were created at about same time as master start and that have no content in them?

          Should we check for regionserver znodes and if present, wait a little longer? Give them another chance?

          Show
          stack added a comment - So though the server is online i will not be able to use the hLog and i get filenotfoundException. Master should not take over hlogs that were created at about same time as master start and that have no content in them? Should we check for regionserver znodes and if present, wait a little longer? Give them another chance?
          Hide
          ramkrishna.s.vasudevan added a comment -

          Yes the RS is slow in checking in.
          The problem here is the master will do the split of the newly checked in RS as he is the RS that is not there in the online list.

          Set<ServerName> onlineServers = new HashSet<ServerName>(serverManager
                  .getOnlineServers().keySet());
              // TODO: Should do this in background rather than block master startup
              status.setStatus("Splitting logs after master startup");
              splitLogAfterStartup(this.fileSystemManager, onlineServers);
          

          To split if i find any log folder which is not belonging to any of those in 'onlineServers'(the online list is already got) we will call split log and finally delete the log folder. So though the server is online i will not be able to use the hLog and i get filenotfoundException. Sorry if am not clear.

          Show
          ramkrishna.s.vasudevan added a comment - Yes the RS is slow in checking in. The problem here is the master will do the split of the newly checked in RS as he is the RS that is not there in the online list. Set<ServerName> onlineServers = new HashSet<ServerName>(serverManager .getOnlineServers().keySet()); // TODO: Should do this in background rather than block master startup status.setStatus( "Splitting logs after master startup" ); splitLogAfterStartup( this .fileSystemManager, onlineServers); To split if i find any log folder which is not belonging to any of those in 'onlineServers'(the online list is already got) we will call split log and finally delete the log folder. So though the server is online i will not be able to use the hLog and i get filenotfoundException. Sorry if am not clear.
          Hide
          stack added a comment -

          Tell us more Ram. If new regionserver comes in and we split its logs, so what? It is not carrying any regions, right? We'll assign it regions when done splitting? Or are you talking about a case where a regionserver is just very slow about checking in?

          Show
          stack added a comment - Tell us more Ram. If new regionserver comes in and we split its logs, so what? It is not carrying any regions, right? We'll assign it regions when done splitting? Or are you talking about a case where a regionserver is just very slow about checking in?
          Hide
          ramkrishna.s.vasudevan added a comment -

          Upped the priority of this defect. While master is coming up

          1->Wait for region server to register
          2->Get the online server list
          3-> Start splitting the logs
          

          Between step 2 and 3 if another new region server registers, we just split the logs of the new region server and in fact delete the HLog folder for that new region server. This seems critical.
          While analysing the issue for which this JIRA was created we ended up in this problem.

          Show
          ramkrishna.s.vasudevan added a comment - Upped the priority of this defect. While master is coming up 1->Wait for region server to register 2->Get the online server list 3-> Start splitting the logs Between step 2 and 3 if another new region server registers, we just split the logs of the new region server and in fact delete the HLog folder for that new region server. This seems critical. While analysing the issue for which this JIRA was created we ended up in this problem.
          Hide
          ramkrishna.s.vasudevan added a comment -

          RS restart here is using kill -9 and then starting it.

          Show
          ramkrishna.s.vasudevan added a comment - RS restart here is using kill -9 and then starting it.

            People

            • Assignee:
              ramkrishna.s.vasudevan
              Reporter:
              ramkrishna.s.vasudevan
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development