[
https://issues.apache.org/jira/browse/HBASE-457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12571210#action_12571210
]
stack commented on HBASE-457:
-----------------------------
Here's a few comments on the patch:
+ This seems odd in MetaScanner: 'synchronized
(master.regionManager.metaScannerLock) ' Would think classes should never have
this kind of intimate knowledge of others: i.e. MetaScanner knowing that master
has a data member named regionManager which in turn has an object to lock. Or
this kinda thing: 'master.regionManager.onlineMetaRegions' or
'master.regionManager.metaRegionsToScan.pol' (I understand this is first stage
of an extensive refactoring and that these once-inner classes are tough to
unravel but maybe something can be done about this intimacy?). This problem
seems prevalent throughout the patch.
+ Can these changes:
{code}
- master.onlineMetaRegions.remove(info.getStartKey());
+ master.regionManager.onlineMetaRegions.remove(info.getStartKey());
{code}
be replace with a method on master named something like
removeOnlineMetaRegion(final Text startKey)?
One suggestion would be to make a getRegionManager method for the master (Are
these supposed to be singletons?) and then a removeOnlineMetaRegions method on
RegionManager.
Same for these in ServerManager:
{code}
+ HServerAddress root = master.getRootRegionLocation();
+ if (root != null && root.equals(storedInfo.getServerAddress())) {
+ master.regionManager.unassignRootRegion();
+ }
+ master.delayedToDoQueue.put(new ProcessServerShutdown(master,
storedInfo));
{code}
+ I like the ServerManager and RegionManager classes
+ Can ServerManager and RegionManager be finals on master? Can they be
private? (smile)
+ Just remove this kinda stuff rather than leave it commented in:
{code}
+/* synchronized (master.regionManager.onlineMetaRegions) {
+ metaRegion = master.regionManager.onlineMetaRegions.size() == 1 ?
+
master.regionManager.onlineMetaRegions.get(master.regionManager.onlineMetaRegions.firstKey())
:
+
master.regionManager.onlineMetaRegions.containsKey(regionInfo.getRegionName()) ?
+
master.regionManager.onlineMetaRegions.get(regionInfo.getRegionName()) :
+
master.regionManager.onlineMetaRegions.get(master.regionManager.onlineMetaRegions.headMap(
regionInfo.getRegionName()).lastKey());
- }
- this.metaRegionName = metaRegion.getRegionName();
+ }*/
{code}
There is another example of above in SortedSoftMap.
Otherwise, patch is definetly going in right direction.
> Factor Master into Master, RegionManager, and ServerManager
> -----------------------------------------------------------
>
> Key: HBASE-457
> URL: https://issues.apache.org/jira/browse/HBASE-457
> Project: Hadoop HBase
> Issue Type: Sub-task
> Components: master
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: 457-v2.patch, 457.patch
>
>
> Even with TableOperation and descendants factored out of HMaster, it's still
> a huge class. Every bit of master state basically lives in one class, so it's
> very challenging to understand the logical groupings of everything.
> To make this a little more manageable, let's make two new abstractions,
> ServerManager and RegionManager.
> ServerManager keeps track of servers - leases, message processing, load and
> load average, etc.
> RegionManager keeps track of the root location, meta table online state,
> assigning regions to servers, and so on.
> HMaster then keeps around one of each of these classes to track state and do
> it's master stuff. The HMaster class itself does not change it's interface to
> external consumers. It also retains the main processing loop, HBase closed
> state, start and stop, etc.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
