[
https://issues.apache.org/jira/browse/HBASE-11290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14132242#comment-14132242
]
Virag Kothari commented on HBASE-11290:
---------------------------------------
Thanks for the review [~mantonov]
bq. LockFreeCache name implies that it's "cache (i.e. structure from where
elements could be evicted without any harm when expired or cache is overfilled)
which is lock free", which isn't exactly how it's used? I mean - the class
itself is suited to serve as a set of named lock objects, and it can't
arbitrarily evict elements from it?
I named it 'lockfree' as we are retrieving elements from cache without
obtaining lock. But seems that is confusing as we are serving lock objects from
cache. How about the name as LockCache? . We need to evict elements otherwise
the map may grow unbounded. In next version, I plan to add Guava's cachebuilder
with soft values instead of the CHM.
bq. to be precise, not just setters updating multiple collections grab region
lock, but readers who want to read consistently from several ones, right? Like
isRegionOnline()? Also this list could contain RW-locks, right? As further
improvements. Now guys like #isRegionOnline() can't run in parallel querying
about the same region from multiple threads, although they could with RW lock.
Yep, for some readers reading several collections we are also grabbing region
locks. Agree that RW-locks can be a future optimization.
bq. for ops like #regionOnline(), does it make sense to have the server-keyed
locks, and operations involving both HRI and server (hence needed to
access/update serverHoldings) would first grab lock for HRI, then lock for
ServerName to work with server holdings? Seems more consistent?
Yeah, I was planning to add server locks initially. But for methods like
regionOnline(), concurrent put and concurrent remove seems to be enough for
server keyed structures like serverHoldings(). Also, we have to be more careful
about deadlock type of situation when we deal with two types of lock (region
and server). So, I thought about avoiding serverlocks for now.
bq. #logSplit made not synchronized now, we don't need locks to iterate over
lastAssignments map or to access processedServers?
The logSplit() is called by SSH and multiple SSH should not call logSplit() for
the same ServerName. Even in a case they do, I dont see any issue with multiple
threads overlapping for this method.
bq. in #serverOffline do we still need notifyAll call at the last lines?
We are doing a wait using object lock somewhere. So kept this as is.
bq. In AM, #addPlan() grabs locks for region name before putting into
regionPlans, but #addPlans() doesn't get any locks? It assumed the lock is
already grabbed for all those regions?
I thought CHM.putAll is atomic but its not the case. Let me see whether we
need locks here.
bq. in #processServerShutdown we don't need locks on regionPlans?
The iterator over regionPlans is weakly consistent and other threads might
update/access the map, but that seems to be ok here.
> Unlock RegionStates
> -------------------
>
> Key: HBASE-11290
> URL: https://issues.apache.org/jira/browse/HBASE-11290
> Project: HBase
> Issue Type: Sub-task
> Reporter: Francis Liu
> Assignee: Virag Kothari
> Attachments: HBASE-11290-0.98.patch, HBASE-11290.draft.patch
>
>
> Even though RegionStates is a highly accessed data structure in HMaster. Most
> of it's methods are synchronized. Which limits concurrency. Even simply
> making some of the getters non-synchronized by using concurrent data
> structures has helped with region assignments. We can go as simple as this
> approach or create locks per region or a bucket lock per region bucket.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
