[
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
