Mailing-List: contact dev-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hbase.apache.org
Received-SPF: pass (nike.apache.org: local policy)
Message-ID: <4C0EE75C.6070709@aconex.com>
Date: Wed, 09 Jun 2010 10:59:08 +1000
From: Paul Cowan <cowan@aconex.com>
Organization: Aconex
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US;
 rv:1.9.1.9) Gecko/20100423 Thunderbird/3.0.4
MIME-Version: 1.0
To: dev@hbase.apache.org
Subject: Re: Review Request: HBASE-2468: Improvements to prewarm META cache
 on 	clients.
References: <20100528211314.13032.44086@ip-10-250-10-165.ec2.internal>
 	<20100528215936.21168.90293@ip-10-250-10-165.ec2.internal>
 	<4C008851.7070807@aconex.com>
 <AANLkTimmub4kUCJ8VlUh5gajqbVn7Tk8DaUcJkZtYLda@mail.gmail.com>
In-Reply-To: <AANLkTimmub4kUCJ8VlUh5gajqbVn7Tk8DaUcJkZtYLda@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi tsuna,

>> Changing this class to use ConcurrentHashMap, and completely removing
>> synchronization, won't work, as it's written. The lazy-loading in
>> regionCachePreFetchEnabled() (get - if null, put) won't work correctly with
>> a ConcurrentHashMap without external synchronization.
>
> I'm not sure to understand why, can you explain?

No problem. With that change, there would be a potential race condition:

Thread 1                                Thread 2
----------                              -------------
enter regionCachePrefetchEnabled
call ConcurrentMap.get() - get null
                                         enter disableRegionCachePrefetch
                                         ConcurrentMap.put(false);
                                         exit disableRegionCachePrefetch
ConcurrentMap.put(true)
exit regionCachePrefetchEnabled


which means that after one (nominally 'read-only') call to 
regionCachePrefetchEnabled and one call to disableRegionCachePrefetch 
have completed, the value in the cache for the table is true, when it 
really should be false.

The get-nullcheck-put triad really needs to be atomic, which requires 
either external synchronization or use of some of the other features of 
ConcurrentHashMap like below.

> Slide 31 of the aforementioned presentation reads "Calls putIfAbsent
> every time it needs to read a value" "Unfortunately, this usage is
> very common" so I think this is not a good idea.
>
> I'm still fairly new to Java so I tend to trust what people like
> Joshua Bloch write, but I can be convinced otherwise :)

That's purely for performance reasons -- you're right, I should have 
suggested
    get, if (result == null) {putIfAbsent, get}
rather than
    putIfAbsent, get

Both are correct but as Josh talks about on that slide (and the 
subsequent one) it's needless contention. I would be almost certain that 
it's still faster than a synchronized block, but not as much as other 
options. Should have mentioned that.

Of the 4 options I mentioned (putIfAbsent(), get() and default return 
value if missing, CopyOnWriteArraySet, ConcurrentHashSet), the 
putIfAbsent() method is likely the slowest (given the usage pattern 
anyway), and (more importantly, in my opinion) the least clear anyway. I 
think the ConcurrentSet solution is much cleaner.

Hope that makes thing clear.

Cheers,

Paul