----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/903/#review1334 -----------------------------------------------------------
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. pom.xml <http://review.cloudera.org/r/903/#comment4450> I'll ignore this change. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java <http://review.cloudera.org/r/903/#comment4452> Should closeAndDelete be added to the WALObserver interface? Could you use the WALObserver for some of your intrusion? src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java <http://review.cloudera.org/r/903/#comment4451> You are introducing white space here src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java <http://review.cloudera.org/r/903/#comment4453> You are introducting white space above and below (Not important... just telling you so you can avoid in future) src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java <http://review.cloudera.org/r/903/#comment4454> What does this class add? Does it change the log splitter code in any way? src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogWriter.java <http://review.cloudera.org/r/903/#comment4455> Try and keep the old formatting... especially as it doesn't look like you changed anything here. - 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 > 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/TestWALReplay.java > 6c11eaf > > Diff: http://review.cloudera.org/r/903/diff > > > Testing > ------- > > > Thanks, > > James > >
