HBase
  1. HBase
  2. HBASE-3305

Allow round-robin distribution for table created with multiple regions

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.20.6
    • Fix Version/s: 0.90.1, 0.92.0
    • Component/s: master
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      We can distribute the initial regions created for a new table in round-robin fashion.

      1. hbase-3305-round-robin-unit-test.patch
        7 kB
        Ted Yu
      2. hbase-3305-default-round-robin.patch
        4 kB
        Ted Yu
      3. hbase-3305-array.patch
        10 kB
        Ted Yu
      4. hbase-3305.patch
        9 kB
        Ted Yu

        Activity

        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #1726 (See https://hudson.apache.org/hudson/job/HBase-TRUNK/1726/)

        Show
        Hudson added a comment - Integrated in HBase-TRUNK #1726 (See https://hudson.apache.org/hudson/job/HBase-TRUNK/1726/ )
        Hide
        Jonathan Gray added a comment -

        Committed to branch and trunk. Thanks Ted, sorry about the long delay!

        Show
        Jonathan Gray added a comment - Committed to branch and trunk. Thanks Ted, sorry about the long delay!
        Hide
        HBase Review Board added a comment -

        Message from: "Jonathan Gray" <jgray@apache.org>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1271/#review2049
        -----------------------------------------------------------

        Ship it!

        looks good, thanks ted! i will commit the final patch to trunk.

        trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <http://review.cloudera.org/r/1271/#comment6456>

        line is > 80 chars but will fix on commit, don't worry

        • Jonathan
        Show
        HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1271/#review2049 ----------------------------------------------------------- Ship it! looks good, thanks ted! i will commit the final patch to trunk. trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < http://review.cloudera.org/r/1271/#comment6456 > line is > 80 chars but will fix on commit, don't worry Jonathan
        Hide
        HBase Review Board added a comment -

        Message from: "Ted Yu" <ted_yu@yahoo.com>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1271/
        -----------------------------------------------------------

        (Updated 2010-12-07 18:28:46.368066)

        Review request for hbase, stack and Jonathan Gray.

        Changes
        -------

        Reverted movement of imports

        Summary
        -------

        Adopted round-robin assignment as default for regions specified when table is created.

        This addresses bug HBASE-3305.
        http://issues.apache.org/jira/browse/HBASE-3305

        Diffs (updated)


        trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1043216
        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1043216
        trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1043216
        trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1043216

        Diff: http://review.cloudera.org/r/1271/diff

        Testing
        -------

        Put unit tests for this change inside TestAdmin.testCreateTableWithRegions()
        They passed.

        Thanks,

        Ted

        Show
        HBase Review Board added a comment - Message from: "Ted Yu" <ted_yu@yahoo.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1271/ ----------------------------------------------------------- (Updated 2010-12-07 18:28:46.368066) Review request for hbase, stack and Jonathan Gray. Changes ------- Reverted movement of imports Summary ------- Adopted round-robin assignment as default for regions specified when table is created. This addresses bug HBASE-3305 . http://issues.apache.org/jira/browse/HBASE-3305 Diffs (updated) trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1043216 trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1043216 trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1043216 trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1043216 Diff: http://review.cloudera.org/r/1271/diff Testing ------- Put unit tests for this change inside TestAdmin.testCreateTableWithRegions() They passed. Thanks, Ted
        Hide
        HBase Review Board added a comment -

        Message from: "Ted Yu" <ted_yu@yahoo.com>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1271/
        -----------------------------------------------------------

        (Updated 2010-12-07 18:25:05.129171)

        Review request for hbase, stack and Jonathan Gray.

        Changes
        -------

        I used Organize Imports in Eclipse for AssignmentManager

        Summary
        -------

        Adopted round-robin assignment as default for regions specified when table is created.

        This addresses bug HBASE-3305.
        http://issues.apache.org/jira/browse/HBASE-3305

        Diffs (updated)


        trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1043216
        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1043216
        trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1043216
        trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1043216

        Diff: http://review.cloudera.org/r/1271/diff

        Testing
        -------

        Put unit tests for this change inside TestAdmin.testCreateTableWithRegions()
        They passed.

        Thanks,

        Ted

        Show
        HBase Review Board added a comment - Message from: "Ted Yu" <ted_yu@yahoo.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1271/ ----------------------------------------------------------- (Updated 2010-12-07 18:25:05.129171) Review request for hbase, stack and Jonathan Gray. Changes ------- I used Organize Imports in Eclipse for AssignmentManager Summary ------- Adopted round-robin assignment as default for regions specified when table is created. This addresses bug HBASE-3305 . http://issues.apache.org/jira/browse/HBASE-3305 Diffs (updated) trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1043216 trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1043216 trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1043216 trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1043216 Diff: http://review.cloudera.org/r/1271/diff Testing ------- Put unit tests for this change inside TestAdmin.testCreateTableWithRegions() They passed. Thanks, Ted
        Hide
        HBase Review Board added a comment -

        Message from: "Ted Yu" <ted_yu@yahoo.com>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1271/#review2048
        -----------------------------------------------------------

        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <http://review.cloudera.org/r/1271/#comment6455>

        I wrap InterruptedException in IOException.

        • Ted
        Show
        HBase Review Board added a comment - Message from: "Ted Yu" <ted_yu@yahoo.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1271/#review2048 ----------------------------------------------------------- trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/1271/#comment6455 > I wrap InterruptedException in IOException. Ted
        Hide
        HBase Review Board added a comment -

        Message from: "Jonathan Gray" <jgray@apache.org>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1271/#review2047
        -----------------------------------------------------------

        almost

        trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <http://review.cloudera.org/r/1271/#comment6451>

        why is this and above import of EventType moved in your diff?

        trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <http://review.cloudera.org/r/1271/#comment6452>

        white space here and two lines below

        trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <http://review.cloudera.org/r/1271/#comment6453>

        put back the previous comment about round-robin, and whitespace (tabs?)

        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <http://review.cloudera.org/r/1271/#comment6454>

        Generally it's not good or "right" to catch, log, and ignore an IE. How is this handled elsewhere?

        • Jonathan
        Show
        HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1271/#review2047 ----------------------------------------------------------- almost trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < http://review.cloudera.org/r/1271/#comment6451 > why is this and above import of EventType moved in your diff? trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < http://review.cloudera.org/r/1271/#comment6452 > white space here and two lines below trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < http://review.cloudera.org/r/1271/#comment6453 > put back the previous comment about round-robin, and whitespace (tabs?) trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/1271/#comment6454 > Generally it's not good or "right" to catch, log, and ignore an IE. How is this handled elsewhere? Jonathan
        Hide
        HBase Review Board added a comment -

        Message from: "Ted Yu" <ted_yu@yahoo.com>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1271/
        -----------------------------------------------------------

        (Updated 2010-12-07 16:56:46.150530)

        Review request for hbase, stack and Jonathan Gray.

        Changes
        -------

        Added AssignmentManager.assignUserRegions() which is called from createTable() and assignAllUserRegions()

        Summary
        -------

        Adopted round-robin assignment as default for regions specified when table is created.

        This addresses bug HBASE-3305.
        http://issues.apache.org/jira/browse/HBASE-3305

        Diffs (updated)


        trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1043216
        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1043216
        trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1043216
        trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1043216

        Diff: http://review.cloudera.org/r/1271/diff

        Testing
        -------

        Put unit tests for this change inside TestAdmin.testCreateTableWithRegions()
        They passed.

        Thanks,

        Ted

        Show
        HBase Review Board added a comment - Message from: "Ted Yu" <ted_yu@yahoo.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1271/ ----------------------------------------------------------- (Updated 2010-12-07 16:56:46.150530) Review request for hbase, stack and Jonathan Gray. Changes ------- Added AssignmentManager.assignUserRegions() which is called from createTable() and assignAllUserRegions() Summary ------- Adopted round-robin assignment as default for regions specified when table is created. This addresses bug HBASE-3305 . http://issues.apache.org/jira/browse/HBASE-3305 Diffs (updated) trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1043216 trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1043216 trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1043216 trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1043216 Diff: http://review.cloudera.org/r/1271/diff Testing ------- Put unit tests for this change inside TestAdmin.testCreateTableWithRegions() They passed. Thanks, Ted
        Hide
        HBase Review Board added a comment -

        Message from: "Jonathan Gray" <jgray@apache.org>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1271/#review2042
        -----------------------------------------------------------

        Almost there. Some spacing only changes still in here and need to move out logic into AM method.

        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <http://review.cloudera.org/r/1271/#comment6445>

        still tabbing changes here and next method signature as well

        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <http://review.cloudera.org/r/1271/#comment6446>

        same as stack's original comment. this logic should be in AssignmentManager. I wouldn't reuse the method 'assignAllUserRegions' because it says "all" in it. A method 'assignUserRegions' which takes a list and does a bulk assign w/ round-robin would make sense . 'assignAllUserRegions' could then call it once it makes a list of regions.

        • Jonathan
        Show
        HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1271/#review2042 ----------------------------------------------------------- Almost there. Some spacing only changes still in here and need to move out logic into AM method. trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/1271/#comment6445 > still tabbing changes here and next method signature as well trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/1271/#comment6446 > same as stack's original comment. this logic should be in AssignmentManager. I wouldn't reuse the method 'assignAllUserRegions' because it says "all" in it. A method 'assignUserRegions' which takes a list and does a bulk assign w/ round-robin would make sense . 'assignAllUserRegions' could then call it once it makes a list of regions. Jonathan
        Hide
        HBase Review Board added a comment -

        Message from: "Ted Yu" <ted_yu@yahoo.com>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1271/
        -----------------------------------------------------------

        (Updated 2010-12-06 23:22:29.259676)

        Review request for hbase, stack and Jonathan Gray.

        Changes
        -------

        Removes tabs.
        Format code using multiple of two spaces.

        Summary
        -------

        Adopted round-robin assignment as default for regions specified when table is created.

        This addresses bug HBASE-3305.
        http://issues.apache.org/jira/browse/HBASE-3305

        Diffs (updated)


        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1042922
        trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1042922
        trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1042922

        Diff: http://review.cloudera.org/r/1271/diff

        Testing
        -------

        Put unit tests for this change inside TestAdmin.testCreateTableWithRegions()
        They passed.

        Thanks,

        Ted

        Show
        HBase Review Board added a comment - Message from: "Ted Yu" <ted_yu@yahoo.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1271/ ----------------------------------------------------------- (Updated 2010-12-06 23:22:29.259676) Review request for hbase, stack and Jonathan Gray. Changes ------- Removes tabs. Format code using multiple of two spaces. Summary ------- Adopted round-robin assignment as default for regions specified when table is created. This addresses bug HBASE-3305 . http://issues.apache.org/jira/browse/HBASE-3305 Diffs (updated) trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1042922 trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1042922 trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1042922 Diff: http://review.cloudera.org/r/1271/diff Testing ------- Put unit tests for this change inside TestAdmin.testCreateTableWithRegions() They passed. Thanks, Ted
        Hide
        HBase Review Board added a comment -

        Message from: "Ted Yu" <ted_yu@yahoo.com>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1271/#review2038
        -----------------------------------------------------------

        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <http://review.cloudera.org/r/1271/#comment6438>

        A new patch will be uploaded that reverts such changes.

        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <http://review.cloudera.org/r/1271/#comment6439>

        I think you're implying rewriting
        AssignmentManager.assignAllUserRegions().

        How about creating this method:
        assignAllUserRegions(List<HRegionInfo> regions).

        finishInitialization() would pass null to the above method to indicate that all user regions should be assigned.
        createTable() would pass the list of regions for the new table.

        This way, BulkStartupAssigner doesn't appear in HMaster.

        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <http://review.cloudera.org/r/1271/#comment6440>

        Yes. I prefer space between if and left parenthesis.

        I will revert anyway.

        • Ted
        Show
        HBase Review Board added a comment - Message from: "Ted Yu" <ted_yu@yahoo.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1271/#review2038 ----------------------------------------------------------- trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/1271/#comment6438 > A new patch will be uploaded that reverts such changes. trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/1271/#comment6439 > I think you're implying rewriting AssignmentManager.assignAllUserRegions(). How about creating this method: assignAllUserRegions(List<HRegionInfo> regions). finishInitialization() would pass null to the above method to indicate that all user regions should be assigned. createTable() would pass the list of regions for the new table. This way, BulkStartupAssigner doesn't appear in HMaster. trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/1271/#comment6440 > Yes. I prefer space between if and left parenthesis. I will revert anyway. Ted
        Hide
        HBase Review Board added a comment -

        Message from: stack@duboce.net

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1271/#review2037
        -----------------------------------------------------------

        Ship it!

        Looks good Ted. Below are a few pointers mostly on formatting and then a few questions. Thanks for making the patch.

        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <http://review.cloudera.org/r/1271/#comment6430>

        Do you need to pollute HMaster with this AssignmentManager inner class?

        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <http://review.cloudera.org/r/1271/#comment6431>

        FYI, don't make these kinda formatting changes in a patch... its distracting and the change you are making is against the convention used in the rest of this file. Just FYI. No biggie.

        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <http://review.cloudera.org/r/1271/#comment6432>

        Yeah, maybe these lines belong inside a method that is inside AssignmentManager? What you think Ted?

        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <http://review.cloudera.org/r/1271/#comment6433>

        What changed on this line? White space?

        trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java
        <http://review.cloudera.org/r/1271/#comment6434>

        Convention is two spaces for tab in hbase and hadoop. This seems like something else?

        trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        <http://review.cloudera.org/r/1271/#comment6435>

        FYI, tab is two spaces... we indent in multiples of two spaces.

        trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        <http://review.cloudera.org/r/1271/#comment6437>

        Good. Nice test.

        • stack
        Show
        HBase Review Board added a comment - Message from: stack@duboce.net ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1271/#review2037 ----------------------------------------------------------- Ship it! Looks good Ted. Below are a few pointers mostly on formatting and then a few questions. Thanks for making the patch. trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/1271/#comment6430 > Do you need to pollute HMaster with this AssignmentManager inner class? trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/1271/#comment6431 > FYI, don't make these kinda formatting changes in a patch... its distracting and the change you are making is against the convention used in the rest of this file. Just FYI. No biggie. trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/1271/#comment6432 > Yeah, maybe these lines belong inside a method that is inside AssignmentManager? What you think Ted? trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/1271/#comment6433 > What changed on this line? White space? trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java < http://review.cloudera.org/r/1271/#comment6434 > Convention is two spaces for tab in hbase and hadoop. This seems like something else? trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java < http://review.cloudera.org/r/1271/#comment6435 > FYI, tab is two spaces... we indent in multiples of two spaces. trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java < http://review.cloudera.org/r/1271/#comment6437 > Good. Nice test. stack
        Hide
        HBase Review Board added a comment -

        Message from: "Ted Yu" <ted_yu@yahoo.com>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1271/
        -----------------------------------------------------------

        (Updated 2010-12-06 10:42:26.792838)

        Review request for hbase, stack and Jonathan Gray.

        Changes
        -------

        Add hbase group as reviewer

        Summary
        -------

        Adopted round-robin assignment as default for regions specified when table is created.

        This addresses bug HBASE-3305.
        http://issues.apache.org/jira/browse/HBASE-3305

        Diffs


        trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1042725
        trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1042725
        trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1042725

        Diff: http://review.cloudera.org/r/1271/diff

        Testing
        -------

        Put unit tests for this change inside TestAdmin.testCreateTableWithRegions()
        They passed.

        Thanks,

        Ted

        Show
        HBase Review Board added a comment - Message from: "Ted Yu" <ted_yu@yahoo.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1271/ ----------------------------------------------------------- (Updated 2010-12-06 10:42:26.792838) Review request for hbase, stack and Jonathan Gray. Changes ------- Add hbase group as reviewer Summary ------- Adopted round-robin assignment as default for regions specified when table is created. This addresses bug HBASE-3305 . http://issues.apache.org/jira/browse/HBASE-3305 Diffs trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1042725 trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1042725 trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1042725 Diff: http://review.cloudera.org/r/1271/diff Testing ------- Put unit tests for this change inside TestAdmin.testCreateTableWithRegions() They passed. Thanks, Ted
        Hide
        Ted Yu added a comment -

        Piggyback unit test on TestAdmin.testCreateTableWithRegions()

        Show
        Ted Yu added a comment - Piggyback unit test on TestAdmin.testCreateTableWithRegions()
        Hide
        Jonathan Gray added a comment -

        Added Ted as contributor and assigning.

        Show
        Jonathan Gray added a comment - Added Ted as contributor and assigning.
        Hide
        Jonathan Gray added a comment -

        Ted, you want to put this patch up on review board? Looks pretty close to being done but a few small comments. You can reuse that numServers variable in the LoadBalancer method rather than calling .size() every time. Get rid of the "there are x servers online" debug message.

        Is there a unit test for the round-robin method? Want to write on? It's a simple algorithm but always good to have something to detect regressions.

        Also, looks like you have tabs in the diff. HBase style is 2 spaces and no tabs.

        Show
        Jonathan Gray added a comment - Ted, you want to put this patch up on review board? Looks pretty close to being done but a few small comments. You can reuse that numServers variable in the LoadBalancer method rather than calling .size() every time. Get rid of the "there are x servers online" debug message. Is there a unit test for the round-robin method? Want to write on? It's a simple algorithm but always good to have something to detect regressions. Also, looks like you have tabs in the diff. HBase style is 2 spaces and no tabs.
        Hide
        Ted Yu added a comment -

        Rewrote roundRobinAssignment() with two loops - the outer loop for iterating servers and the inner for iterating over regions.

        Show
        Ted Yu added a comment - Rewrote roundRobinAssignment() with two loops - the outer loop for iterating servers and the inner for iterating over regions.
        Hide
        Ted Yu added a comment -

        This patch makes round-robin default assignment when creating tables.

        Show
        Ted Yu added a comment - This patch makes round-robin default assignment when creating tables.
        Hide
        Ted Yu added a comment -

        If the total number of regions is small compared to the number of region servers, the benefit of round-robin is less obvious.
        Our use case would do round-robin assignment for every table we create. I have no problem making this the default.

        I plan to move the initialization of mRandomizer out of roundRobinAssignment() for better randomness.

        Show
        Ted Yu added a comment - If the total number of regions is small compared to the number of region servers, the benefit of round-robin is less obvious. Our use case would do round-robin assignment for every table we create. I have no problem making this the default. I plan to move the initialization of mRandomizer out of roundRobinAssignment() for better randomness.
        Hide
        Jonathan Gray added a comment -

        I would rather not complicate the API and introduce new enums all the way to the client. I see no reason why you'd want random assignment not round-robin w/ random starting point, so I see no reason to make this configurable (at least on a per create table call basis). If anything, make it a Configuration parameter but I don't even think that's necessary.

        Show
        Jonathan Gray added a comment - I would rather not complicate the API and introduce new enums all the way to the client. I see no reason why you'd want random assignment not round-robin w/ random starting point, so I see no reason to make this configurable (at least on a per create table call basis). If anything, make it a Configuration parameter but I don't even think that's necessary.
        Hide
        Ted Yu added a comment -

        Third version randomizes the starting region server in LoadBalancer.roundRobinAssignment()

        Show
        Ted Yu added a comment - Third version randomizes the starting region server in LoadBalancer.roundRobinAssignment()
        Hide
        Ted Yu added a comment -

        I wasn't able to get RPC working using EnumSet.
        Here is another version that uses arrays in place of EnumSet.

        I exposed the TableCreationFlag to client so that client has better control over the distribution of regions.

        I will attach another patch which changes the starting server for roundRobinAssignment().

        Show
        Ted Yu added a comment - I wasn't able to get RPC working using EnumSet. Here is another version that uses arrays in place of EnumSet. I exposed the TableCreationFlag to client so that client has better control over the distribution of regions. I will attach another patch which changes the starting server for roundRobinAssignment().
        Hide
        Ted Yu added a comment -

        I wasn't able to get RPC working using EnumSet.
        Here is another version that uses arrays in place of EnumSet.

        I exposed the TableCreationFlag to client so that client has better control over the distribution of regions.

        Show
        Ted Yu added a comment - I wasn't able to get RPC working using EnumSet. Here is another version that uses arrays in place of EnumSet. I exposed the TableCreationFlag to client so that client has better control over the distribution of regions.
        Hide
        Jonathan Gray added a comment -

        Seems reasonable to me that we'd have this as the default rather than all of the new option enum stuff. When would you not want round robin? I think the one thing to change in the round robin assignment is to start at a random starting point.

        Show
        Jonathan Gray added a comment - Seems reasonable to me that we'd have this as the default rather than all of the new option enum stuff. When would you not want round robin? I think the one thing to change in the round robin assignment is to start at a random starting point.
        Hide
        Ted Yu added a comment -

        First attempt for this JIRA.

        Show
        Ted Yu added a comment - First attempt for this JIRA.
        Hide
        Ted Yu added a comment -

        Currently we have this in HMaster:

          private synchronized void createTable(final HRegionInfo [] newRegions, boolean sync)
        

        Maybe we should change the second parameter to EnumSet<TableCreationFlag> so that both sync and round-robin options can be expressed.

        Show
        Ted Yu added a comment - Currently we have this in HMaster: private synchronized void createTable( final HRegionInfo [] newRegions, boolean sync) Maybe we should change the second parameter to EnumSet<TableCreationFlag> so that both sync and round-robin options can be expressed.

          People

          • Assignee:
            Ted Yu
            Reporter:
            Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development