You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Ted Yu <te...@yahoo.com> on 2010/12/06 19:42:26 UTC
Re: Review Request: HBASE-3305 Allow round-robin distribution for table
created with multiple regions
-----------------------------------------------------------
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
Re: Review Request: HBASE-3305 Allow round-robin distribution for table
created with multiple regions
Posted by st...@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
On 2010-12-06 10:42:26, Ted Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1271/
> -----------------------------------------------------------
>
> (Updated 2010-12-06 10:42:26)
>
>
> Review request for hbase, stack and Jonathan Gray.
>
>
> 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
>
>
Re: Review Request: HBASE-3305 Allow round-robin distribution for table
created with multiple regions
Posted by Jonathan Gray <jg...@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
On 2010-12-07 16:56:46, Ted Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1271/
> -----------------------------------------------------------
>
> (Updated 2010-12-07 16:56:46)
>
>
> Review request for hbase, stack and Jonathan Gray.
>
>
> 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/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
>
>
Re: Review Request: HBASE-3305 Allow round-robin distribution for table
created with multiple regions
Posted by Ted Yu <te...@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
On 2010-12-07 16:56:46, Ted Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1271/
> -----------------------------------------------------------
>
> (Updated 2010-12-07 16:56:46)
>
>
> Review request for hbase, stack and Jonathan Gray.
>
>
> 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/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
>
>
Re: Review Request: HBASE-3305 Allow round-robin distribution for
table created with multiple regions
Posted by Ted Yu <yu...@gmail.com>.
Any chance that this JIRA makes to the next 0.90 RC ?
Thanks
On Tue, Dec 7, 2010 at 7:06 PM, Jonathan Gray <jg...@apache.org> wrote:
>
> -----------------------------------------------------------
> 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
>
>
> On 2010-12-07 18:28:46, Ted Yu wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > http://review.cloudera.org/r/1271/
> > -----------------------------------------------------------
> >
> > (Updated 2010-12-07 18:28:46)
> >
> >
> > Review request for hbase, stack and Jonathan Gray.
> >
> >
> > 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/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
> >
> >
>
>
Re: Review Request: HBASE-3305 Allow round-robin distribution for table
created with multiple regions
Posted by Jonathan Gray <jg...@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
On 2010-12-07 18:28:46, Ted Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1271/
> -----------------------------------------------------------
>
> (Updated 2010-12-07 18:28:46)
>
>
> Review request for hbase, stack and Jonathan Gray.
>
>
> 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/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
>
>
Re: Review Request: HBASE-3305 Allow round-robin distribution for table
created with multiple regions
Posted by Ted Yu <te...@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
Re: Review Request: HBASE-3305 Allow round-robin distribution for table
created with multiple regions
Posted by Ted Yu <te...@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
Re: Review Request: HBASE-3305 Allow round-robin distribution for table
created with multiple regions
Posted by Ted Yu <te...@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
Re: Review Request: HBASE-3305 Allow round-robin distribution for table
created with multiple regions
Posted by Jonathan Gray <jg...@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
On 2010-12-06 23:22:29, Ted Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1271/
> -----------------------------------------------------------
>
> (Updated 2010-12-06 23:22:29)
>
>
> Review request for hbase, stack and Jonathan Gray.
>
>
> 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 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
>
>
Re: Review Request: HBASE-3305 Allow round-robin distribution for table
created with multiple regions
Posted by Ted Yu <te...@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
Re: Review Request: HBASE-3305 Allow round-robin distribution for table
created with multiple regions
Posted by Ted Yu <te...@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
On 2010-12-06 10:42:26, Ted Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1271/
> -----------------------------------------------------------
>
> (Updated 2010-12-06 10:42:26)
>
>
> Review request for hbase, stack and Jonathan Gray.
>
>
> 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
>
>