HBase
  1. HBase
  2. HBASE-5992

Generalization of region move implementation + manage draining servers in bulk assign

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.95.2
    • Fix Version/s: 0.95.0
    • Component/s: master, regionserver
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The region move implementation now has now a similar behavior whatever the destination server is specified or not. This allows:

      • to benefit from the improvement in HBASE-5877
      • as a side effect to have the coprocessors calls when the destination server is not specified

      This includes various fixes around draining servers. Draining servers were not excluded during a bulk assign. This is now fixed.

      1. 5992.v5.patch
        26 kB
        Nicolas Liochon
      2. 5992.v5.patch
        26 kB
        Ted Yu
      3. 5992.v2.patch
        25 kB
        Nicolas Liochon
      4. 5992.v11.patch
        26 kB
        Nicolas Liochon

        Issue Links

          Activity

          Hide
          stack added a comment -

          FYI, you don't need to change the style here:

          -      if (rst.isAlive()) liveServers.add(rst);
          -      else LOG.info("Not alive " + rst.getName());
          +      if (rst.isAlive()){
          +        liveServers.add(rst);
          +      }
          

          Its ok having no curlies if statement fits one line.

          In hbase code base, we have spaces around operators... i.e. this needs spaces

           "+encodedName+" 

          The refactoring that makes results int this method,

          +    final List<ServerName> destServers = serverManager.createDestinationServersList(serverToExclude);

          looks good.

          Ditto for this change

          +    this.serverManager.removeDeadNotExpiredServers(servers);

          moving 'server' state out of AM and up into SM where it belongs.

          Ugh. There was a bunch of dup'd code that this patch removes.

          createDestinationServersList and removeDeadNotExpiredServers do not need to be public methods (oh, hang on, createDestinationServersList needs to be so accessible from handlers.... does removeDeadNotExpiredServers also need to be?)

          What does the big refactoring of TestDrainingServer do?

          Patch looks good to me.

          Show
          stack added a comment - FYI, you don't need to change the style here: - if (rst.isAlive()) liveServers.add(rst); - else LOG.info( "Not alive " + rst.getName()); + if (rst.isAlive()){ + liveServers.add(rst); + } Its ok having no curlies if statement fits one line. In hbase code base, we have spaces around operators... i.e. this needs spaces "+encodedName+" The refactoring that makes results int this method, + final List<ServerName> destServers = serverManager.createDestinationServersList(serverToExclude); looks good. Ditto for this change + this .serverManager.removeDeadNotExpiredServers(servers); moving 'server' state out of AM and up into SM where it belongs. Ugh. There was a bunch of dup'd code that this patch removes. createDestinationServersList and removeDeadNotExpiredServers do not need to be public methods (oh, hang on, createDestinationServersList needs to be so accessible from handlers.... does removeDeadNotExpiredServers also need to be?) What does the big refactoring of TestDrainingServer do? Patch looks good to me.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12526565/5992.v2.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.regionserver.TestSplitTransactionOnCluster
          org.apache.hadoop.hbase.TestDrainingServer

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1851//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1851//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1851//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/12526565/5992.v2.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.regionserver.TestSplitTransactionOnCluster org.apache.hadoop.hbase.TestDrainingServer Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1851//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1851//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1851//console This message is automatically generated.
          Hide
          Nicolas Liochon added a comment -

          What does the big refactoring of TestDrainingServer do?

          Hopefully it's now non flaky. Before both the test and the core code were a little bit flaky! .

          In hbase code base, we have spaces around operators... i.e. this needs spaces

          Done.

          does removeDeadNotExpiredServers also need to be?

          It does compile if it's package protected. I didn't dare doing it in the first patch to maintain the existing interface, but since you're asking . Done.

          TestDrainingServer failed for an unrelated reason in its setup. But well. Fixed (hopefully, I actually don't really reproduce the issue locally) as well in the next patch.

          Show
          Nicolas Liochon added a comment - What does the big refactoring of TestDrainingServer do? Hopefully it's now non flaky. Before both the test and the core code were a little bit flaky! . In hbase code base, we have spaces around operators... i.e. this needs spaces Done. does removeDeadNotExpiredServers also need to be? It does compile if it's package protected. I didn't dare doing it in the first patch to maintain the existing interface, but since you're asking . Done. TestDrainingServer failed for an unrelated reason in its setup. But well. Fixed (hopefully, I actually don't really reproduce the issue locally) as well in the next patch.
          Hide
          Ted Yu added a comment -
          -    this.servers.entrySet();
          +    this.servers.entrySet();   // todo: ?? very smart or copy paste error
          

          Can we remove the above statement ?

          In HMaster.java:

          +        if (this.cpHost.preMove( hri , rp.getSource(), rp.getDestination())) {
          

          Please remove the spaces around hri - minor.

          +   * Creates a list of possible destination for a region. It contains the online servers, but not
          

          'destination' -> 'destinations'

          +  public List<ServerName> createDestinationServersList(){
          

          Please add javadoc for the method above.

          Show
          Ted Yu added a comment - - this .servers.entrySet(); + this .servers.entrySet(); // todo: ?? very smart or copy paste error Can we remove the above statement ? In HMaster.java: + if ( this .cpHost.preMove( hri , rp.getSource(), rp.getDestination())) { Please remove the spaces around hri - minor. + * Creates a list of possible destination for a region. It contains the online servers, but not 'destination' -> 'destinations' + public List<ServerName> createDestinationServersList(){ Please add javadoc for the method above.
          Hide
          Nicolas Liochon added a comment -

          v5 with Stack's and Ted's comments taken into account. Thanks.

          Show
          Nicolas Liochon added a comment - v5 with Stack's and Ted's comments taken into account. Thanks.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12526590/5992.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 27 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:

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1854//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1854//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1854//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/12526590/5992.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 27 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: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1854//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1854//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1854//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Re-attaching patch v5.

          Show
          Ted Yu added a comment - Re-attaching patch v5.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12526617/5992.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 27 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:

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1858//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1858//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1858//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/12526617/5992.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 27 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: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1858//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1858//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1858//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Still cannot find the hanging test from Jenkins console.
          I am running test suite on Linux.

          If there is no hanging test there, will integrate the patch.

          Show
          Ted Yu added a comment - Still cannot find the hanging test from Jenkins console. I am running test suite on Linux. If there is no hanging test there, will integrate the patch.
          Hide
          Nicolas Liochon added a comment -

          It's org.apache.hadoop.hbase.TestDrainingServer.
          I fixed the issue, I'm retesting before uploading the patch.

          Show
          Nicolas Liochon added a comment - It's org.apache.hadoop.hbase.TestDrainingServer. I fixed the issue, I'm retesting before uploading 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/12526647/5992.v11.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 appears to introduce 31 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.TestSplitLogManager

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1863//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1863//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1863//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/12526647/5992.v11.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 appears to introduce 31 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.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1863//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1863//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1863//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          I looped TestDrainingServer 4 times.

          TestSplitLogManager passes locally on MacBook.

          Integrated to trunk.

          Thanks for the patch, N.

          Thanks for the review, Stack.

          Show
          Ted Yu added a comment - I looped TestDrainingServer 4 times. TestSplitLogManager passes locally on MacBook. Integrated to trunk. Thanks for the patch, N. Thanks for the review, Stack.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2881 (See https://builds.apache.org/job/HBase-TRUNK/2881/)
          HBASE-5992 Generalization of region move implementation + manage draining servers in bulk assign (N Keywal) (Revision 1337641)

          Result = SUCCESS
          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/CreateTableHandler.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2881 (See https://builds.apache.org/job/HBase-TRUNK/2881/ ) HBASE-5992 Generalization of region move implementation + manage draining servers in bulk assign (N Keywal) (Revision 1337641) Result = SUCCESS tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/CreateTableHandler.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #3 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/3/)
          HBASE-5992 Generalization of region move implementation + manage draining servers in bulk assign (N Keywal) (Revision 1337641)

          Result = FAILURE
          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/CreateTableHandler.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #3 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/3/ ) HBASE-5992 Generalization of region move implementation + manage draining servers in bulk assign (N Keywal) (Revision 1337641) Result = FAILURE tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/CreateTableHandler.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
          Hide
          stack added a comment -

          Marking closed.

          Show
          stack added a comment - Marking closed.

            People

            • Assignee:
              Nicolas Liochon
              Reporter:
              Nicolas Liochon
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development