hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "James Kennedy" <jk-pub...@troove.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 17:08:51 GMT

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

(Updated 2010-09-27 10:08:51.004233)

Review request for hbase.


Addressed Stack's review comments.
Mostly just formatting fixes but also impled the WALObserver change as suggested.


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.

Diffs (updated)

  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/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




View raw message