Minor Improvements related to balancer.

Details

Description

Currently in Am.getAssignmentByTable() we use a result map which is currenly a hashmap. It could be better if we have a treeMap. Even in MetaReader.fullScan we have the treeMap only so that we have the naming order maintained. I felt this change could be very useful in cases where we are extending the DefaultLoadBalancer.

Activity

There are few more changes that we can do here
-> Currently two different balancer objects are created in AssignmentManager and HMaster. Unify them
-> balancer.randomAssignment() is getting called once but based on whether the existingplan is null or if it is a force plan we either use the randomplan or not.
Ideally if we are extending a new balancer then randomAssignment will be called but later the plan given by randomAssignment may not be used.

ramkrishna.s.vasudevan
added a comment - 10/Apr/12 13:57 There are few more changes that we can do here
-> Currently two different balancer objects are created in AssignmentManager and HMaster. Unify them
-> balancer.randomAssignment() is getting called once but based on whether the existingplan is null or if it is a force plan we either use the randomplan or not.
Ideally if we are extending a new balancer then randomAssignment will be called but later the plan given by randomAssignment may not be used.

Ted Yu
added a comment - 10/Apr/12 17:47 For AssignmentManager.setBalancer(), there is whitespace in javadoc.
'balancer to set' should be on the same line as @param line.
w.r.t. introducing TreeMap, I would prefer to see the benefit in a real world scenario. I think HashMap serves us well currently.

We use treemap. Here in getAssignmentsByTable we try to give the table to region, server mapping.
The places where we do rebuildUserRegions also we try to maintain the order as retrieved from the meta.
Here also we try to iterate the 'servers' tree map and form the result. Hence i felt using a tree map will maintain some consistency.
And for some real world scenario, if i have a pair of tables may be named TableA and TableA_xxxx then when i want to do a balancing for these two tables thro an extended balancer there is a chance that TableA_xxxx comes first and then TableA. But this use case is very specific.

ramkrishna.s.vasudevan
added a comment - 10/Apr/12 18:05 @Zhihong
Thanks for your review.
If you see the code base where we deal with the regions assignment like the
private final NavigableMap<ServerName, Set<HRegionInfo>> servers =
new TreeMap<ServerName, Set<HRegionInfo>>();
private final SortedMap<HRegionInfo, ServerName> regions =
new TreeMap<HRegionInfo, ServerName>();
We use treemap. Here in getAssignmentsByTable we try to give the table to region, server mapping.
The places where we do rebuildUserRegions also we try to maintain the order as retrieved from the meta.
Here also we try to iterate the 'servers' tree map and form the result. Hence i felt using a tree map will maintain some consistency.
And for some real world scenario, if i have a pair of tables may be named TableA and TableA_xxxx then when i want to do a balancing for these two tables thro an extended balancer there is a chance that TableA_xxxx comes first and then TableA. But this use case is very specific.

ramkrishna.s.vasudevan
added a comment - 10/Apr/12 18:07 - edited Pls review the latest one. If you feel still TreeMap change is not needed, i think the other changes are needed.
{edit}
Pls review the latest one. If you feel still TreeMap change is not needed, i think the other changes are needed like
this .balancer.setMasterServices( this );
This is needed because as part of join cluster we call assign(). If we extend the load balancer and want to use the masterservices we will not be able to use it. Hence the change is needed.{edit}

-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 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.

Hadoop QA
added a comment - 10/Apr/12 18:25 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12522135/HBASE-5737.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 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1465//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1465//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1465//console
This message is automatically generated.

There is one more whitespace @ line 2986.
Please keep HashMap for now. For the case of two tables cited above, there is time limit on the actual region movement per balance() call. Meaning the balancing of the two tables may be completed in several iterations.

Ted Yu
added a comment - 10/Apr/12 18:28 There is one more whitespace @ line 2986.
Please keep HashMap for now. For the case of two tables cited above, there is time limit on the actual region movement per balance() call. Meaning the balancing of the two tables may be completed in several iterations.

-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 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.

Hadoop QA
added a comment - 10/Apr/12 18:42 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12522142/HBASE-5737_1.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 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.master.TestAssignmentManager
Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1467//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1467//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1467//console
This message is automatically generated.

-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 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.

Hadoop QA
added a comment - 11/Apr/12 18:26 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12522284/HBASE-5737_2.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 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 1 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/1480//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1480//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1480//console
This message is automatically generated.

Ram, the AM#setBalancer is not right? Doesn't AM make a balancer instance of its own up in its constructor? We should at least remove that. Could we pass in the load balancer to use into the AM's constructor rather than call a setBalancer method?

stack
added a comment - 12/Apr/12 05:15 Ram, the AM#setBalancer is not right? Doesn't AM make a balancer instance of its own up in its constructor? We should at least remove that. Could we pass in the load balancer to use into the AM's constructor rather than call a setBalancer method?

@Stack
I prefer passing as constructor but that needs changes in many places.
Actually i had 2 ways of doing this. But the TestAssignmentManager was failing if we remove the balancer in the AssignmentManager.

ramkrishna.s.vasudevan
added a comment - 12/Apr/12 08:17 @Stack
I prefer passing as constructor but that needs changes in many places.
Actually i had 2 ways of doing this. But the TestAssignmentManager was failing if we remove the balancer in the AssignmentManager.
Though the changes are many passing in constructor should be fine.

Hadoop QA
added a comment - 15/Apr/12 18:40 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12522713/HBASE-5737_3.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 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 3 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/1533//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1533//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1533//console
This message is automatically generated.

ramkrishna.s.vasudevan
added a comment - 19/Apr/12 04:16 - edited @Stack
- this .balancer = LoadBalancerFactory.getLoadBalancer(conf);
+ this .balancer = balancer;
The above change seems to a bug to me. That is why i asked whether it is an improvement or bug?

stack
added a comment - 19/Apr/12 05:15 The above is from AM? If so, I'm not sure it a bug. At the time, my sense is that the balancer ran w/o keeping context. Whats changed is that you seem to have a LB that is doing this now.
As to whether a bug or improvement, its your call boss.

ramkrishna.s.vasudevan
added a comment - 19/Apr/12 08:36 @Stack
Yes i have a configured LB. But as we provide option to use master services in the LB and now if i try to use the 'balancer' object there, it is a new one. I am updating it to a bug Stack.