HBase
  1. HBase
  2. HBASE-7271

Have a single executor for all zkWorkers in the assignment manager

    Details

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

      Description

      The current strategy is to have an array of monothreaded executor, and hash the zk path to ensure that there are no two events on the same region executed in parallel

      I think a single executor, as presented in the attached patch, is better because:

      • we're guaranteed to use all threads at any time
      • if managing one of the event takes longer that expected, the slowness is limited to this region, and not to all regions that have the same hashed/moduloed code
      • For the nodeChildrenChanged, there is no need to choose randomly one of the worker (or, once again, the risk to get stuck if one of the event takes time to be managed).
      1. 7271.v1.patch
        8 kB
        Nicolas Liochon
      2. 7271.v2.patch
        9 kB
        Nicolas Liochon
      3. 7271.v2.patch
        9 kB
        Nicolas Liochon
      4. 7271.v3.patch
        9 kB
        Nicolas Liochon

        Issue Links

          Activity

          Hide
          stack added a comment -

          Assigning Jimmy for his opinion on nkeyway's proposed change.

          Show
          stack added a comment - Assigning Jimmy for his opinion on nkeyway's proposed change.
          Hide
          Jimmy Xiang added a comment -

          Looked at the patch. The idea is great.

          +  // In a multimap, the put order is kept when we retrieve the collection back. We need this
          +  //  as we want the events to be managed in the same order as we received them.
          

          Are we sure the order is kept? It seems to me HashMultiMap uses just a regular HashSet to store the values which don't keep the order.
          If so, we can just use a hash map from region name to a linked list of RegionRunnable instead.

          Show
          Jimmy Xiang added a comment - Looked at the patch. The idea is great. + // In a multimap, the put order is kept when we retrieve the collection back. We need this + // as we want the events to be managed in the same order as we received them. Are we sure the order is kept? It seems to me HashMultiMap uses just a regular HashSet to store the values which don't keep the order. If so, we can just use a hash map from region name to a linked list of RegionRunnable instead.
          Hide
          Jimmy Xiang added a comment -

          Good idea, nkeyway! Back to you.

          Show
          Jimmy Xiang added a comment - Good idea, nkeyway! Back to you.
          Hide
          Ted Yu added a comment -
          +  // We want don't want to have two events on the same region managed simultaneously.
          

          Remove the first 'want' above.

          +  private final Set<String> inProgress = new HashSet<String>();
          

          There're many actions in progress. How about naming the above variable regionsInProgress ?

          +  private final Multimap<String, RegionRunnable> todo =  HashMultimap.create()
          

          Would zkEventWorkerWaitingList be a better name for todo ?
          nit: remove one space before HashMultimap.

          +  protected void zkEventWorkersSubmit(final RegionRunnable regRunnable) {
          +    synchronized (inProgress) {
          +      if (inProgress.contains(regRunnable.getRegionName())) {
          +        synchronized (todo){
          +          todo.put(regRunnable.getRegionName(), regRunnable);
          +        }
          +      } else {
          

          nit: if you return from if block, you can save one indent for else block where else keyword can be omitted.

          Show
          Ted Yu added a comment - + // We want don't want to have two events on the same region managed simultaneously. Remove the first 'want' above. + private final Set< String > inProgress = new HashSet< String >(); There're many actions in progress. How about naming the above variable regionsInProgress ? + private final Multimap< String , RegionRunnable> todo = HashMultimap.create() Would zkEventWorkerWaitingList be a better name for todo ? nit: remove one space before HashMultimap. + protected void zkEventWorkersSubmit( final RegionRunnable regRunnable) { + synchronized (inProgress) { + if (inProgress.contains(regRunnable.getRegionName())) { + synchronized (todo){ + todo.put(regRunnable.getRegionName(), regRunnable); + } + } else { nit: if you return from if block, you can save one indent for else block where else keyword can be omitted.
          Hide
          Ted Yu added a comment -

          If so, we can just use a hash map from region name to a linked list of RegionRunnable instead.

          LinkedHashMultimap would give us the ordering guarantee. See its javadoc:

           * Implementation of {@code Multimap} that does not allow duplicate key-value
           * entries and that returns collections whose iterators follow the ordering in
           * which the data was added to the multimap.
          
          Show
          Ted Yu added a comment - If so, we can just use a hash map from region name to a linked list of RegionRunnable instead. LinkedHashMultimap would give us the ordering guarantee. See its javadoc: * Implementation of {@code Multimap} that does not allow duplicate key-value * entries and that returns collections whose iterators follow the ordering in * which the data was added to the multimap.
          Hide
          Jimmy Xiang added a comment -

          +1 for LinkedHashMultimap

          Show
          Jimmy Xiang added a comment - +1 for LinkedHashMultimap
          Hide
          Ted Yu added a comment -
          +  static interface RegionRunnable extends Runnable{
          

          The above interface is only used within AssignmentManager - it can be made private.

          Show
          Ted Yu added a comment - + static interface RegionRunnable extends Runnable { The above interface is only used within AssignmentManager - it can be made private.
          Hide
          Nicolas Liochon added a comment -

          I'm ok will all the comments, I will provide an updated version tomorrow, my time.

          Show
          Nicolas Liochon added a comment - I'm ok will all the comments, I will provide an updated version tomorrow, my time.
          Hide
          Nicolas Liochon added a comment -

          v2, with all comments taken into accounts. I fixed two unrelated nits unrelated to my patch as well. Local tests ok.

          Show
          Nicolas Liochon added a comment - v2, with all comments taken into accounts. I fixed two unrelated nits unrelated to my patch as well. Local tests ok.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12555949/7271.v1.patch
          against trunk revision .

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          -1 javadoc. The javadoc tool appears to have generated 101 warning messages.

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

          -1 findbugs. The patch appears to introduce 23 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/3451//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//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/12555949/7271.v1.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 101 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 23 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/3451//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3451//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Patch v2 looks good. Following can be addressed on commit.

          +    int workers = conf.getInt("hbase.assignment.zkevent.workers", 20);
          

          nit: why the increase in number of workers ?

          +            // now that we have finished, let see if there is an event for the same region in the
          

          nit: 'let see' -> "let's see"

          Show
          Ted Yu added a comment - Patch v2 looks good. Following can be addressed on commit. + int workers = conf.getInt( "hbase.assignment.zkevent.workers" , 20); nit: why the increase in number of workers ? + // now that we have finished, let see if there is an event for the same region in the nit: 'let see' -> "let's see"
          Hide
          Nicolas Liochon added a comment -

          nit: why the increase in number of workers ?

          It's far from nit . I think 5 is not enough. I've done tests bumping the number of threads everywhere in HBASE-7248. I will redo it after once HBASE-7247 & HBASE-7246 are committed, so identify something reasonably optimal, but I wanted to put a first stone there...

          Show
          Nicolas Liochon added a comment - nit: why the increase in number of workers ? It's far from nit . I think 5 is not enough. I've done tests bumping the number of threads everywhere in HBASE-7248 . I will redo it after once HBASE-7247 & HBASE-7246 are committed, so identify something reasonably optimal, but I wanted to put a first stone there...
          Hide
          stack added a comment -

          On commit, perhaps change thread name to have hosting servername prefix (see other thread namings).

          The check in the below should be under the synchronization block too...

          + if (regionsInProgress.contains(regRunnable.getRegionName())) {
          + synchronized (zkEventWorkerWaitingList)

          { + zkEventWorkerWaitingList.put(regRunnable.getRegionName(), regRunnable); + }

          + return;
          + }

          Can do on commit.

          +1 on commit.

          Show
          stack added a comment - On commit, perhaps change thread name to have hosting servername prefix (see other thread namings). The check in the below should be under the synchronization block too... + if (regionsInProgress.contains(regRunnable.getRegionName())) { + synchronized (zkEventWorkerWaitingList) { + zkEventWorkerWaitingList.put(regRunnable.getRegionName(), regRunnable); + } + return; + } Can do on commit. +1 on commit.
          Hide
          Jimmy Xiang added a comment - - edited

          On commit, probably set the iterator to a local variable, and call iterator.remove() instead.

          +                  RegionRunnable next = waiting.iterator().next();
          +                  zkEventWorkerWaitingList.remove(next.getRegionName(), next);
          
          Show
          Jimmy Xiang added a comment - - edited On commit, probably set the iterator to a local variable, and call iterator.remove() instead. + RegionRunnable next = waiting.iterator().next(); + zkEventWorkerWaitingList.remove(next.getRegionName(), next);
          Hide
          Nicolas Liochon added a comment -

          v3. Testing locally.
          All comments above taken into account, excepted:

          The check in the below should be under the synchronization block too...

          The code as it is is ok imho?

          On commit, probably set the iterator to a local variable, and call iterator.remove() instead.

          Guava should return something nicer than a simple Set, so it's not perfect whatever we do... I added some comments and renamed a variable.

          Show
          Nicolas Liochon added a comment - v3. Testing locally. All comments above taken into account, excepted: The check in the below should be under the synchronization block too... The code as it is is ok imho? On commit, probably set the iterator to a local variable, and call iterator.remove() instead. Guava should return something nicer than a simple Set, so it's not perfect whatever we do... I added some comments and renamed a variable.
          Hide
          stack added a comment -

          The check in the below should be under the synchronization block too...

          Doing the check outside of the sync block and then doing the put inside the sync block allows that the backing map can change between the check and put. Why not move the check under the sync too?

          Show
          stack added a comment - The check in the below should be under the synchronization block too... Doing the check outside of the sync block and then doing the put inside the sync block allows that the backing map can change between the check and put. Why not move the check under the sync too?
          Hide
          Nicolas Liochon added a comment -

          We've got two lists, hence two locks: we keep the lock on the first one:

          +    synchronized (regionsInProgress) {
          +      // If we're there is already a task with this region, we add it to the
          +      //  waiting list and return.
          +      if (regionsInProgress.contains(regRunnable.getRegionName())) {
          +        synchronized (zkEventWorkerWaitingList){
          +          zkEventWorkerWaitingList.put(regRunnable.getRegionName(), regRunnable);
          +        }
          +        return;
                   ====> We won't touch anymore zkEventWorkerWaitingList, we can release the lock
          +      }
          +
          ====> We still have the lock on regionsInProgress
          +      // No event in progress on this region => we can submit a new task immediately.
          +      regionsInProgress.add(regRunnable.getRegionName());
          
          Show
          Nicolas Liochon added a comment - We've got two lists, hence two locks: we keep the lock on the first one: + synchronized (regionsInProgress) { + // If we're there is already a task with this region, we add it to the + // waiting list and return . + if (regionsInProgress.contains(regRunnable.getRegionName())) { + synchronized (zkEventWorkerWaitingList){ + zkEventWorkerWaitingList.put(regRunnable.getRegionName(), regRunnable); + } + return ; ====> We won't touch anymore zkEventWorkerWaitingList, we can release the lock + } + ====> We still have the lock on regionsInProgress + // No event in progress on this region => we can submit a new task immediately. + regionsInProgress.add(regRunnable.getRegionName());
          Hide
          Nicolas Liochon added a comment -

          Committed v3, I will amend (or revert ) if it's necessary.

          Show
          Nicolas Liochon added a comment - Committed v3, I will amend (or revert ) if it's necessary.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #3604 (See https://builds.apache.org/job/HBase-TRUNK/3604/)
          HBASE-7271 Have a single executor for all zkWorkers in the assignment manager (Revision 1419351)

          Result = FAILURE
          nkeywal :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #3604 (See https://builds.apache.org/job/HBase-TRUNK/3604/ ) HBASE-7271 Have a single executor for all zkWorkers in the assignment manager (Revision 1419351) Result = FAILURE nkeywal : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          Hide
          stack added a comment -

          Sorry. I was reading the contains check as being done on the zkEventWorkerWaitingList list. +1 on commit.

          Show
          stack added a comment - Sorry. I was reading the contains check as being done on the zkEventWorkerWaitingList list. +1 on commit.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #291 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/291/)
          HBASE-7271 Have a single executor for all zkWorkers in the assignment manager (Revision 1419351)

          Result = FAILURE
          nkeywal :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #291 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/291/ ) HBASE-7271 Have a single executor for all zkWorkers in the assignment manager (Revision 1419351) Result = FAILURE nkeywal : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          Hide
          Nicolas Liochon added a comment -

          Thanks for the reviews. Resolving...

          Show
          Nicolas Liochon added a comment - Thanks for the reviews. Resolving...
          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:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development