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