----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5073/#review7720 -----------------------------------------------------------
Hari, thank you for the patch! I don't plan on doing the full review since I don't have the cycles right now, but I saw this come up right before bed so I thought I would add my input. flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java <https://reviews.apache.org/r/5073/#comment17006> Wish: It'd be ideal if we could use mockito to pass it a fake HTable object and then test that transactions are handled correctly if Error and RuntimeException are thrown. flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java <https://reviews.apache.org/r/5073/#comment17005> super.stop(); ? flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java <https://reviews.apache.org/r/5073/#comment17004> Most components are taking CamelCap parameters. https://github.com/apache/flume/blob/trunk/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java <https://reviews.apache.org/r/5073/#comment17001> Maybe we should use a different default row key? I am guessing the row key prefix is supposed to be used to get around hot spotting due to the timestamp. Maybe UUID would be a better default? flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java <https://reviews.apache.org/r/5073/#comment17002> Should we specify the UTF-8 charset here? flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java <https://reviews.apache.org/r/5073/#comment17003> We are just dropping the headers altogether? I think we should insert them as cells (a flag could turn this off). flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java <https://reviews.apache.org/r/5073/#comment16998> I don't plan on doing the major review of this, but Throwable should be caught here and rollback called. If the throwable is Error or RuntimeException it should be thrown without being wrapped. - Brock On 2012-05-09 03:04:07, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5073/ > ----------------------------------------------------------- > > (Updated 2012-05-09 03:04:07) > > > Review request for Flume. > > > Summary > ------- > > Hbase sink. > > > This addresses bug FLUME-1183. > https://issues.apache.org/jira/browse/FLUME-1183 > > > Diffs > ----- > > > flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestHBaseSink.java > PRE-CREATION > flume-ng-sinks/pom.xml acb3087 > pom.xml 8c11a2d > flume-ng-dist/pom.xml 5bdcfe7 > flume-ng-sinks/flume-ng-hbase-sink/pom.xml PRE-CREATION > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java > PRE-CREATION > bin/flume-ng 0108997 > > Diff: https://reviews.apache.org/r/5073/diff > > > Testing > ------- > > Unit tests added > > > Thanks, > > Hari > >
