hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Todd Lipcon" <t...@cloudera.com>
Subject Re: Review Request: Batch writes should sync to HLog in batches
Date Fri, 11 Jun 2010 17:23:51 GMT


> On 2010-06-11 09:59:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1439
> > <http://review.hbase.org/r/167/diff/1/?file=1237#file1237line1439>
> >
> >     Should this be public?  Isn't it just used internally?

good call, will make it package private


> On 2010-06-11 09:59:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1442
> > <http://review.hbase.org/r/167/diff/1/?file=1237#file1237line1442>
> >
> >     Is this a copy?

Yep. I did it the simple way following the pseudocode in the JIRA, but we can definitely do
it more efficiently by indexing into a list or arraylist. My bet is that the efficiency gains
are marginal compared to the cost of actually writing to the WAL, etc, and this patch should
already be a big gain over what we used to have.

After this is committed let's file a followup JIRA to remove extra shallow copies. I'll add
a TODO in the code also.


> On 2010-06-11 09:59:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1467
> > <http://review.hbase.org/r/167/diff/1/?file=1237#file1237line1467>
> >
> >     You were going to replace these w/ something from guava (or is this it?)

The guava Preconditions thing is supposed to be more for parameter checking, etc, than for
internal assertions. Currently these don't get run, but I think we should enable -ea at least
for unit tests.


> On 2010-06-11 09:59:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1482
> > <http://review.hbase.org/r/167/diff/1/?file=1237#file1237line1482>
> >
> >     This creates new Map, pass in Map.Entry instead?

I thought both .keySet() and .entrySet() were just views into the existing map? (and p.getFamilyMap()
just returns a member)


> On 2010-06-11 09:59:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1777
> > <http://review.hbase.org/r/167/diff/1/?file=1237#file1237line1777>
> >
> >     w can never be null here?  (There was null check previous)

yea, since it's assigned first in the try block, and that function doesn't throw exceptions.


- Todd


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


On 2010-06-11 00:50:05, Todd Lipcon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/167/
> -----------------------------------------------------------
> 
> (Updated 2010-06-11 00:50:05)
> 
> 
> Review request for hbase, Kannan Muthukkaruppan and Ryan Rawson.
> 
> 
> Summary
> -------
> 
> I implemented the "mini batching" idea we talked about on the JIRA.
> 
> This currently breaks some of the error handling, so I dont intend to commit as is, but
everyone is busy so wanted to put a review up now while I tidy up the rest.
> 
> 
> This addresses bug HBASE-2353.
>     http://issues.apache.org/jira/browse/HBASE-2353
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6b6d098 
>   src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java a1baff4 
>   src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 034690e 
> 
> Diff: http://review.hbase.org/r/167/diff
> 
> 
> Testing
> -------
> 
> Some PEs on a real sync-enabled cluster, seems faster but haven't done scientific benchmarking.
> 
> 
> Thanks,
> 
> Todd
> 
>


Mime
View raw message