hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@duboce.net
Subject Re: Review Request: HBASE-2641 Refactor HLog splitLog, hbase-2437 continued; break out split code as new classes
Date Fri, 24 Sep 2010 21:54:08 GMT

This is an automatically generated e-mail. To reply, visit:

Patch looks good to me.  Take a look at the few small issues below James.  See what you think.
  Otherwise, I'm game for commit.  Don't worry about the different ways of Writer/Reader creation
for now I'd say.


    I'll ignore this change.


    Should closeAndDelete be added to the WALObserver interface?  Could you use the WALObserver
for some of your intrusion?


    You are introducing white space here


    You are introducting white space above and below (Not important... just telling you so
you can avoid in future)


    What does this class add?  Does it change the log splitter code in any way?


    Try and keep the old formatting... especially as it doesn't look like you changed anything

- stack

On 2010-09-24 10:21:25, James Kennedy wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/903/
> -----------------------------------------------------------
> (Updated 2010-09-24 10:21:25)
> Review request for hbase.
> Summary
> -------
> Moved HLog split code into an HLogSplitter.  This patch was necessary for the hbase-transactional-indexed
project to be able to properly extend various HBase classes to facilitate the management of
a separate THLog.  As such, the patch also focuses on making some code more extensible.
> Stack has already done a preliminary review of this patch and gave the comment:
> "Do we have one way of overriding Writer -- e.g. in subclass provide
> override of createWriterInstance -- but a different way making Readers
> -- by setting class name to instantiate into the Configuration?  If
> so, should be one way only.... I don't care which."
> I seek advice on how to remedy this.  We added HLog.createWriterInstance() because we
needed our THLog extension of HLog to be able to setup it's own SequenceFileLogWriter to use
the special THLogKey yet still have HLog use the writer given in configuration property. 
Since HLog.rollWriter() needs to instantiate a writer inside HLog, using the createWriterInstance()
template method seemed the easiest choice but that is the only place it is ever called from.
 We did not need to do the same for the THLog reader because there are no non-static HLog
calls to create a reader. Everywhere we read THLog it would suffice to use the static THLog.getReader()
method which uses a THLogKey reader.
> So I see how this asymmetry is unpleasant. We could add an HLog.createReaderInstance()
and replace all the static calls everywhere WHERE POSSIBLE. But it isn't possible from ALL
places anyway since sometimes a reader is fetched before an HLog instance if available.  So
we need the static accessors anyway...
> This addresses bug 2641.
>     http://issues.apache.org/jira/browse/2641
> Diffs
> -----
>   pom.xml 267ab49 
>   src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java d870d44 
>   src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java fdef130 
>   src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java f2e4e7c 
>   src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java bb3b382 
>   src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 2ebcdf2 
>   src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java PRE-CREATION

>   src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogReader.java 7883fcd

>   src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogWriter.java bb71bed

>   src/test/java/org/apache/hadoop/hbase/regionserver/wal/InstrumentedSequenceFileLogWriter.java
>   src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java 7111776 
>   src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogMethods.java ba6514a

>   src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java 976876c 
>   src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplay.java 6c11eaf 
> Diff: http://review.cloudera.org/r/903/diff
> Testing
> -------
> Thanks,
> James

View raw message