> On 2012-05-09 05:04:12, Brock Noland wrote: > > 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.
Thanks for you comments, Brock! I will make sure that these are addressed in the next patch, which I will submit after someone does a full review. > On 2012-05-09 05:04:12, Brock Noland wrote: > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java, > > line 132 > > <https://reviews.apache.org/r/5073/diff/1/?file=107970#file107970line132> > > > > 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. I am not too familiar with mockito. I will do that in a different patch, when I have time to pick up Mockito. > On 2012-05-09 05:04:12, Brock Noland wrote: > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java, > > line 176 > > <https://reviews.apache.org/r/5073/diff/1/?file=107970#file107970line176> > > > > super.stop(); ? Oops! Will add in the next patch. > On 2012-05-09 05:04:12, Brock Noland wrote: > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java, > > line 182 > > <https://reviews.apache.org/r/5073/diff/1/?file=107970#file107970line182> > > > > 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 I was not actually sure, will change this. > On 2012-05-09 05:04:12, Brock Noland wrote: > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java, > > line 231 > > <https://reviews.apache.org/r/5073/diff/1/?file=107970#file107970line231> > > > > 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? The idea is that the user gives an initial prefix in the conf. This way they can supply different prefixes for different sinks, within the same agent(and later identify which sink each of the rows came from). I agree that using uuid is a better default, but the concerns I have are its size, and also that scans will return the rows in a different order than inserted, while inserting it using timestamps will guarantee that values inserted in a specific order will be returned together. I would like your feedback on that, if that is not a major use case, then I will change it to uuid, since the implementation is also cleaner. Please let me know. > On 2012-05-09 05:04:12, Brock Noland wrote: > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java, > > line 234 > > <https://reviews.apache.org/r/5073/diff/1/?file=107970#file107970line234> > > > > Should we specify the UTF-8 charset here? will do. > On 2012-05-09 05:04:12, Brock Noland wrote: > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java, > > line 239 > > <https://reviews.apache.org/r/5073/diff/1/?file=107970#file107970line239> > > > > We are just dropping the headers altogether? I think we should insert > > them as cells (a flag could turn this off). I am considering using something like gson to convert the event into json and writing that. We can provide a wrapper called Serializable event, which allows serialize and deserialize. The event serializer is available, but it is not a good fit, and it also expects an output stream. I don't want to separate the headers and body while writing to the hbase. Using different cells would do this. Easiest if we can take the byte array form hbase and then convert it back to the json and return the headers and body. > On 2012-05-09 05:04:12, Brock Noland wrote: > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java, > > line 263 > > <https://reviews.apache.org/r/5073/diff/1/?file=107970#file107970line263> > > > > 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. Will do. - Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5073/#review7720 ----------------------------------------------------------- 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 > >
