hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "stack (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-6109) Improve RIT performances during assignment on large clusters
Date Mon, 28 May 2012 22:44:23 GMT

    [ https://issues.apache.org/jira/browse/HBASE-6109?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13284565#comment-13284565
] 

stack commented on HBASE-6109:
------------------------------

@nkeywal Trunk should have settled now.  Can you redo your patch so its against the hbase
root dir?

{code}+  final private Locker locker = new Locker();{code}

Is this a generic locker?  Should it be named for what its locking?

NotifiableConcurrentSkipListMap needs class comment.  It seems like its for use in a very
particular circumstance.  It needs explaining.

Does it need to be public?  Only used in master package?   Perhaps make it package private
then?

internalList is a bad name for the internal delegate instance.  Is 'delegatee' a better name
than internalList?

For sure this is ok?

{code}
+    while (!this.master.isStopped() &&
+      // no lock concurrent access ok: sequentially consistent
+      this.regionsInTransition.containsKey(hri.getEncodedName())) {
+      this.regionsInTransition.waitForListUpdate();
     }
{code}

We checked rit contains a name but then in a separate statement we do the waitForListUpdate?
 What if the region we are looking for is removed between the check and the waitForListUpdate
invocation?

Will this log be annoying?

{code}
+      LOG.info("regionState=" + regionState +
+        " failoverProcessedRegions.containsKey(encodedRegionName)=" + failoverProcessedRegions.containsKey(encodedRegionName));
{code}

This too... '+      LOG.info("et=" + et);'?

.. and this '+        LOG.info("regionInfo.isMetaTable()=" + regionInfo.isMetaTable());'?

Add the region removed to the log message here? +      LOG.debug("Removed region from reopening
regions because it was closed");?

Sometimes your indents are off.  For example:

{code}
-    synchronized (regionsInTransition) {
+      // We need a lock here as we're going to do a put later and we don't want multiple
state
+      //  creation
+    Reentran....
{code}

There are gratuitious reformattings of code.

Is this true:

{code}
+      // no lock concurrency ok: there is a write when we update the timestamp but it's ok
+      //  as its the only one updating this field
+      RegionState rs = this.regionsInTransition.get(e.getKey());
{code}

How is it enforced?

Check these...

{code}

+    }finally {



 or here


+      }else {

{code}

needs space after curly parens.  Sometimes you do it and sometimes you don't.



I reviewed half of the patch.

It looks great.  Nice stuff N.
                
> Improve RIT performances during assignment on large clusters
> ------------------------------------------------------------
>
>                 Key: HBASE-6109
>                 URL: https://issues.apache.org/jira/browse/HBASE-6109
>             Project: HBase
>          Issue Type: Improvement
>          Components: master
>    Affects Versions: 0.96.0
>            Reporter: nkeywal
>            Assignee: nkeywal
>            Priority: Minor
>         Attachments: 6109.v7.patch
>
>
> The main points in this patch are:
>  - lowering the number of copy of the RIT list
>  - lowering the number of synchronization
>  - synchronizing on a region rather than on everything
> It also contains:
>  - some fixes around the RIT notification: the list was sometimes modified without a
corresponding 'notify'.
>  - some tests flakiness correction, actually unrelated to this patch.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message