[
https://issues.apache.org/jira/browse/HBASE-457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12571795#action_12571795
]
stack commented on HBASE-457:
-----------------------------
In the below in metascanner...
{code}
+ /** Work for the meta scanner is queued up here */
+ private volatile BlockingQueue<MetaRegion> metaRegionsToScan =
+ new LinkedBlockingQueue<MetaRegion>();
{code}
...change the volatile to final. I think your hope was that the volatile would
apply to the content of metaRegionsToScan. Rather it applies to the
metaRegionsToScan reference... and that ain't going to change once its been
allocated (See
http://java.sun.com/docs/books/jls/third_edition/html/classes.html#8.3.1.4).
You should change the few like the below in ServerManager also:
{code}
+ /** The map of known server names to server info */
+ protected volatile Map<String, HServerInfo> serversToServerInfo =
+ new ConcurrentHashMap<String, HServerInfo>();
{code}
Do data members in ServerManager have to be protected? Can they not be private
or are they need to be protected for tests? E.g:
{code}
+ protected HMaster master;
+
+ protected final Leases serverLeases;
{code}
If need to be protected for tests then lets make an issue to fix this using
something like this,
http://www.onjava.com/pub/a/onjava/2003/11/12/reflection.html, if its not too
much of a pain.
Does only the master have a regionManager? If so, why not
master.getRegionManager and make the method package-private? Then it would not
have to be passed to the MetaScanner constructor? Not important -- passing on
construction saves an indirection every time... just a thought... would help w/
the below from ProcessServerShutdown:
{code}
- master.onlineMetaRegions.remove(info.getStartKey());
+ master.regionManager.offlineMetaRegion(info.getStartKey());
{code}
I like the cleanup done in the below:
{code}
- ArrayList<MetaRegion> regions = new ArrayList<MetaRegion>();
- synchronized (master.onlineMetaRegions) {
- regions.addAll(master.onlineMetaRegions.values());
- }
+ List<MetaRegion> regions = regionManager.getListOfOnlineMetaRegions();
{code}
(Not important), the below is a TODO?
{code}
+ /** compute the average load across all region servers */
+ public int averageLoad() {
+ return 0;
+ }
{code}
I like the new setUnassigned and noLongerPending, getRootRegionLocation, etc.,
methods
Fix the volatiles in RegionManager... make them finals. You can do this now
you declare and define all in the one statement.
I ran the patch locally and it passed. Suggest that you address what you think
important in the above -- at least the volatiles I'd say -- and if tests pass
locally for you, just apply v5.
> 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-v3.patch, 457-v4.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.
