-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/98/
-----------------------------------------------------------

Review request for hbase.
Summary
-------

HBASE-2468: Improvements to prewarm META cache on clients.

Changes:
1. Add new HTable methods which support region info de/serialation, and region cache prewarm:
- void serializeRegionInfo(): clients could perform a large scan for all the meta for the table, serialize the meta to a file. MR job can ship a copy of the meta for the table in the DistributedCache
- Map<HRegionInfo, HServerAddress> deserializeRegionInfo(): MR job can deserialize the region info from the DistributedCache
- prewarmRegionCache(Map<HRegionInfo, HServerAddress> regionMap): MR job can prewarm local region cache by the deserialized region info.

2. For each client, each region cache read-miss could trigger read-ahead some number of rows in META. This option could be turned on and off for one particular table.
This addresses bug HBASE-2468.
    http://issues.apache.org/jira/browse/HBASE-2468
Diffs
-----

  src/main/java/org/apache/hadoop/hbase/client/HConnection.java 853164d
  src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 09de2ac
  src/main/java/org/apache/hadoop/hbase/client/HTable.java 7ec95cb
  src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java 3de661e
  src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 95e494a

Diff: http://review.hbase.org/r/98/diff
Testing
-------

Unit tests passed locally for me.
Thanks,

Mingjie

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/98/
-----------------------------------------------------------

(Updated 2010-05-28 13:20:33.843332)
Review request for hbase.
Changes
-------

Improved a little, to address the comments from Benoit Sigoure, at http://review.hbase.org/r/78/#review91.
Summary
-------

HBASE-2468: Improvements to prewarm META cache on clients.

Changes:
1. Add new HTable methods which support region info de/serialation, and region cache prewarm:
- void serializeRegionInfo(): clients could perform a large scan for all the meta for the table, serialize the meta to a file. MR job can ship a copy of the meta for the table in the DistributedCache
- Map<HRegionInfo, HServerAddress> deserializeRegionInfo(): MR job can deserialize the region info from the DistributedCache
- prewarmRegionCache(Map<HRegionInfo, HServerAddress> regionMap): MR job can prewarm local region cache by the deserialized region info.

2. For each client, each region cache read-miss could trigger read-ahead some number of rows in META. This option could be turned on and off for one particular table.
This addresses bug HBASE-2468.
    http://issues.apache.org/jira/browse/HBASE-2468
Diffs (updated)
-----

  src/main/java/org/apache/hadoop/hbase/client/HConnection.java 853164d
  src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 09de2ac
  src/main/java/org/apache/hadoop/hbase/client/HTable.java 7ec95cb
  src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java 3de661e
  src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 95e494a

Diff: http://review.hbase.org/r/98/diff
Testing
-------

Unit tests passed locally for me.
Thanks,

Mingjie

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/98/#review93
-----------------------------------------------------------

src/main/java/org/apache/hadoop/hbase/client/HConnection.java
<http://review.hbase.org/r/98/#comment531>

    Properly document this argument.

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment534>

    Please use a concurrent collection and remove the synchronized blocks.  For guidance, see around slide 30 of this presentation:
    http://www.cs.umd.edu/class/fall2009/cmsc132H/slides/still-effective.pdf

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment532>

    Coding style: put the catch on the previous line.

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment533>

    We don't typically call methods using `this.methodname(args)' – remove the `this.'

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment535>

    Add a space before the `:'.

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment536>

    Use `Boolean.TRUE' instead of `new Boolean(true)'.

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment537>

    Use `Boolean.FALSE' instead of `new Boolean(false)'.

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment538>

    Use `Boolean.TRUE' instead of `new Boolean(true)'.

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment539>

    Remove the outer parentheses.

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment540>

    Add a space before the `:'.

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment541>

    If this fits on the previous line while staying under 80 columns, please wrap it around.

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment542>

    Remove the space after `cacheLocation'

src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment546>

    Use {@link #readFields readFields} instead of <code>readFields</code>

src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment544>

    You can remove this <p>

src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment543>

    Use <pre> not <code>

src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment545>

    Add a space before the `:'.

src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment547>

    Use {@link #getRegionsInfo getRegionsInfo}

src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment548>

    Use {@link ...} here and below.

src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment550>

    Wrap this around with the previous line.

src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment549>

    I believe you can remove this block.
- Benoit
On 2010-05-28 13:20:33, Mingjie Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/98/
> -----------------------------------------------------------
>
> (Updated 2010-05-28 13:20:33)
>
>
> Review request for hbase.
>
>
> Summary
> -------
>
> HBASE-2468: Improvements to prewarm META cache on clients.
>
> Changes:

> On 2010-05-28 14:13:14, Benoit Sigoure wrote:
> > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 306
> > <http://review.hbase.org/r/98/diff/3/?file=741#file741line306>
> >
> >     Please use a concurrent collection and remove the synchronized blocks.  For guidance, see around slide 30 of this presentation:
> >     http://www.cs.umd.edu/class/fall2009/cmsc132H/slides/still-effective.pdf

Belay that order, I need to inspect this code review... unfortunately a concurrent collection is not the answer to all multi threading woes.
> On 2010-05-28 14:13:14, Benoit Sigoure wrote:
> > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1594
> > <http://review.hbase.org/r/98/diff/3/?file=741#file741line1594>
> >
> >     Use `Boolean.TRUE' instead of `new Boolean(true)'.

Please use the autoboxing features of java6 (our target platform). Ie: just 'true' and 'false' for this and all others.
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/98/#review93
-----------------------------------------------------------
On 2010-05-28 13:20:33, Mingjie Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/98/
> -----------------------------------------------------------
>
> (Updated 2010-05-28 13:20:33)
>
>
> Review request for hbase.
>
>
> Summary
> -------
>
> HBASE-2468: Improvements to prewarm META cache on clients.
>
> Changes:
> 1. Add new HTable methods which support region info de/serialation, and region cache prewarm:
> - void serializeRegionInfo(): clients could perform a large scan for all the meta for the table, serialize the meta to a file. MR job can ship a copy of the meta for the table in the DistributedCache
> - Map<HRegionInfo, HServerAddress> deserializeRegionInfo(): MR job can deserialize the region info from the DistributedCache
> - prewarmRegionCache(Map<HRegionInfo, HServerAddress> regionMap): MR job can prewarm local region cache by the deserialized region info.
>
> 2. For each client, each region cache read-miss could trigger read-ahead some number of rows in META. This option could be turned on and off for one particular table.
>
>
> This addresses bug HBASE-2468.
>     http://issues.apache.org/jira/browse/HBASE-2468
>
>
> Diffs
> -----
>
>   src/main/java/org/apache/hadoop/hbase/client/HConnection.java 853164d
>   src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 09de2ac
>   src/main/java/org/apache/hadoop/hbase/client/HTable.java 7ec95cb
>   src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java 3de661e
>   src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 95e494a
>
> Diff: http://review.hbase.org/r/98/diff
>
>
> Testing
> -------
>
> Unit tests passed locally for me.
>
>
> Thanks,
>
> Mingjie
>
>

[Commenting on-list, not sure if I should just sign up + put these
comments direct on reviews?]

On 29/05/2010 7:59 AM, Ryan Rawson wrote:
 > Belay that order, I need to inspect this code review... unfortunately
a concurrent collection is not the answer to all multi threading woes.

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.

As it is, the class is thread-safe in the way it accesses the HashMap.
To remove the synchronization (if desired), you could:

1) Replace with a ConcurrentHashMap, and change
regionCachePreFetchEnabled() to do something like:
        prefetchRegionCacheEnabled.putIfAbsent(tableName, true);
        return prefetchRegionCacheEnabled.get(tableName);
Alternatively, remove the 'true' -- that's the default, right? So there
isn't actually any reason to put true values in the map if there's no
value otherwise. Then just use:
        Boolean result = this.prefetchRegionCacheEnabled.get(tableName);
        return result == null ? Boolean.TRUE : result;
(and it's simpler, right?)
2) Again, given that 'true' is the default value, there's probably
actually no reason to use a Map at all. All you want to know is which
ones have specifically had fetching disabled, right? So assuming that
disabling/enabling prefetch for a region is rare (both in terms of
frequency, and in terms of a small number of regions being disabled),
the best performance will probably come from something like
       private final Set<byte[]> prefetchDisabledRegions
         = new CopyOnWriteArraySet<byte[]>()
then disable is just:
       prefetchDisabledRegions.add(name);
enable is:
       prefetchDisabledRegions.remove(name);
and isEnabled is just
       prefetchDisabledRegions.contains(name);
which is probably going to be quicker if disabling is done only for a
few regions (so linear walking of the underlying array is quicker than
hashing + probing) -- and remember, scans of CopyOnWriteArraySet are
completely lock-free. Also, it's cleaner IMHO...
3) Use the solution from #2 with a good ConcurrentHashSet (there isn't
one in the JDK, unfortunately, but maybe you have one available), which
is more useful if large numbers of regions will be disabled or if
regions will be toggled on/off frequently.

Cheers,

Paul

PS: Minor suggestion: this class is inconsistent with variable/method
names as to whether 'prefetch' is one camel-cased word (prefetch) or two
(preFetch). Just thought I'd mention it!

Paul.

 > [Commenting on-list, not sure if I should just sign up + put these
 > comments direct on reviews?]

Yes. You're supposed to sign in and write comment directly to
http://review.hbase.org

 > So assuming that
 > disabling/enabling prefetch for a region is rare (both in terms of
 > frequency, and in terms of a small number of regions being disabled),
 > the best performance will probably come from something like
 > private final Set<byte[]> prefetchDisabledRegions
 > = new CopyOnWriteArraySet<byte[]>()

Excellent suggestion. It's the exact use case. I will follow your
suggestion and use CopyOnWriteArraySet instead of HashMap.

Thanks,
Mingjie

On 05/28/2010 08:21 PM, Paul Cowan wrote:
> [Commenting on-list, not sure if I should just sign up + put these
> comments direct on reviews?]
>
> On 29/05/2010 7:59 AM, Ryan Rawson wrote:
>  > Belay that order, I need to inspect this code review... unfortunately
> a concurrent collection is not the answer to all multi threading woes.
>
> 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.
>
> As it is, the class is thread-safe in the way it accesses the HashMap.
> To remove the synchronization (if desired), you could:
>
> 1) Replace with a ConcurrentHashMap, and change
> regionCachePreFetchEnabled() to do something like:
> prefetchRegionCacheEnabled.putIfAbsent(tableName, true);
> return prefetchRegionCacheEnabled.get(tableName);
> Alternatively, remove the 'true' -- that's the default, right? So there
> isn't actually any reason to put true values in the map if there's no
> value otherwise. Then just use:
> Boolean result = this.prefetchRegionCacheEnabled.get(tableName);
> return result == null ? Boolean.TRUE : result;
> (and it's simpler, right?)
>
>
> 2) Again, given that 'true' is the default value, there's probably
> actually no reason to use a Map at all. All you want to know is which
> ones have specifically had fetching disabled, right? So assuming that
> disabling/enabling prefetch for a region is rare (both in terms of
> frequency, and in terms of a small number of regions being disabled),
> the best performance will probably come from something like
> private final Set<byte[]> prefetchDisabledRegions
> = new CopyOnWriteArraySet<byte[]>()
> then disable is just:
> prefetchDisabledRegions.add(name);
> enable is:
> prefetchDisabledRegions.remove(name);
> and isEnabled is just
> prefetchDisabledRegions.contains(name);
> which is probably going to be quicker if disabling is done only for a
> few regions (so linear walking of the underlying array is quicker than
> hashing + probing) -- and remember, scans of CopyOnWriteArraySet are
> completely lock-free. Also, it's cleaner IMHO...
>
>
> 3) Use the solution from #2 with a good ConcurrentHashSet (there isn't
> one in the JDK, unfortunately, but maybe you have one available), which
> is more useful if large numbers of regions will be disabled or if
> regions will be toggled on/off frequently.
>
> Cheers,
>
> Paul
>
> PS: Minor suggestion: this class is inconsistent with variable/method
> names as to whether 'prefetch' is one camel-cased word (prefetch) or two
> (preFetch). Just thought I'd mention it!
>

On Fri, May 28, 2010 at 8:21 PM, Paul Cowan <[EMAIL PROTECTED]> wrote:
> 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?

> 1) Replace with a ConcurrentHashMap, and change regionCachePreFetchEnabled()
> to do something like:
>       prefetchRegionCacheEnabled.putIfAbsent(tableName, true);
>       return prefetchRegionCacheEnabled.get(tableName);

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 :)

--
Benoit "tsuna" Sigoure
Software Engineer @ www.StumbleUpon.com

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

On Tue, Jun 8, 2010 at 5:59 PM, Paul Cowan <[EMAIL PROTECTED]> wrote:
> 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.

Oh, I see.  But that's because the regionCachePrefetchEnabled method
was unnecessarily changing the Map.  If the "read-only" methods like
this one weren't modifying any data structure (and they don't need
to), this problem wouldn't exist.

Also even though ConcurrentHashSet doesn't exist, it can be
implemented by storing some random, unique Object instance as the
value.  I think that's how HashSet is implemented:

public class HashSet<E> [...] {
    [...]
    private transient HashMap<E,Object> map;

    // Dummy value to associate with an Object in the backing Map
    private static final Object PRESENT = new Object();

    [...]
    public boolean add(E e) {
        return map.put(e, PRESENT)==null;
    }

--
Benoit "tsuna" Sigoure
Software Engineer @ www.StumbleUpon.com

On 09/06/10 15:56, tsuna wrote:
> Oh, I see.  But that's because the regionCachePrefetchEnabled method
> was unnecessarily changing the Map.  If the "read-only" methods like
> this one weren't modifying any data structure (and they don't need
> to), this problem wouldn't exist.

Yep, exactly. None of the other 3 ways need additional synchronization,
it's just the one specific way it was implemented (hence my 'won't work,
as it's written' originally).

> Also even though ConcurrentHashSet doesn't exist, it can be
> implemented by storing some random, unique Object instance as the
> value.  I think that's how HashSet is implemented:

Yeah, it's very easily implemented. In fact, there's a utility method to
do this easily (which I only just learned about a little while ago,
after sending the original email -- there's always something new to learn!):

   Collections.newSetFromMap(new ConcurrentHashMap());

which would work correctly in this instance. As I said though, if it's a
write-rarely, read-frequently, low-number-of-entries scenario then
CopyOnWriteArraySet will likely work just as well, if not better.

Cheers,

Paul

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/98/#review95
-----------------------------------------------------------

src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment553>

    What does this method actually do? If its a predicate start it with 'is'.  Also precaching should be off by default...
     Is it?
- Ryan
On 2010-05-28 13:20:33, Mingjie Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/98/
> -----------------------------------------------------------
>
> (Updated 2010-05-28 13:20:33)
>
>
> Review request for hbase.
>
>
> Summary
> -------
>
> HBASE-2468: Improvements to prewarm META cache on clients.
>
> Changes:
> 1. Add new HTable methods which support region info de/serialation, and region cache prewarm:
> - void serializeRegionInfo(): clients could perform a large scan for all the meta for the table, serialize the meta to a file. MR job can ship a copy of the meta for the table in the DistributedCache
> - Map<HRegionInfo, HServerAddress> deserializeRegionInfo(): MR job can deserialize the region info from the DistributedCache
> - prewarmRegionCache(Map<HRegionInfo, HServerAddress> regionMap): MR job can prewarm local region cache by the deserialized region info.
>
> 2. For each client, each region cache read-miss could trigger read-ahead some number of rows in META. This option could be turned on and off for one particular table.
>
>
> This addresses bug HBASE-2468.
>     http://issues.apache.org/jira/browse/HBASE-2468
>
>
> Diffs
> -----
>
>   src/main/java/org/apache/hadoop/hbase/client/HConnection.java 853164d
>   src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 09de2ac
>   src/main/java/org/apache/hadoop/hbase/client/HTable.java 7ec95cb
>   src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java 3de661e
>   src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 95e494a
>
> Diff: http://review.hbase.org/r/98/diff
>
>
> Testing
> -------
>
> Unit tests passed locally for me.
>
>
> Thanks,
>
> Mingjie
>
>