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 Mon, 27 Sep 2010 21:11:58 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/903/#review1341
-----------------------------------------------------------

Ship it!


- stack


On 2010-09-27 10:08:51, James Kennedy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/903/
> -----------------------------------------------------------
> 
> (Updated 2010-09-27 10:08:51)
> 
> 
> 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
> -----
> 
>   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/LogRoller.java a8fba15 
>   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/main/java/org/apache/hadoop/hbase/regionserver/wal/WALObserver.java cc360c4 
>   src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java d3595b5

>   src/test/java/org/apache/hadoop/hbase/regionserver/wal/InstrumentedSequenceFileLogWriter.java
41329c3 
>   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/TestWALObserver.java a1a1881

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


Mime
View raw message