sorry Sean, missed your message, call me anytime. Christy
On Fri, Sep 24, 2010 at 2:54 PM, <st...@duboce.net> wrote: > > ----------------------------------------------------------- > 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 > > > > > >
