----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8864/#review15217 -----------------------------------------------------------
Hari, great patch! A few items below all minor. flume-ng-doc/sphinx/FlumeUserGuide.rst <https://reviews.apache.org/r/8864/#comment32823> This coupled with the statement below about hbase-site.xml are slightly unclear to me. How about something like "Zookkeeper Quorum and parent node configuration may be specified in the flume configuration file, alternatively these configuration values are taken from the first hbase-site.xml file in the classpath. flume-ng-doc/sphinx/FlumeUserGuide.rst <https://reviews.apache.org/r/8864/#comment32824> Great doc update! znodeParent has a default value above, do we need to re-state it's not mandatory? flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8864/#comment32826> Should this constructor delegate to the other constructor passing in a null? flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8864/#comment32822> How about passing in empty string as the default this would eliminate the null check. Also should we be calling .trim() on the return value of getString()? flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8864/#comment32820> Good idea! How about doing same check for base dir and give both checks a good error message? - Brock Noland On Jan. 8, 2013, 12:14 a.m., Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8864/ > ----------------------------------------------------------- > > (Updated Jan. 8, 2013, 12:14 a.m.) > > > Review request for Flume. > > > Description > ------- > > AsyncHbasesink can now be configured to use zk quorum and zk root from > configuration file. > > > This addresses bug FLUME-1821. > https://issues.apache.org/jira/browse/FLUME-1821 > > > Diffs > ----- > > flume-ng-doc/sphinx/FlumeUserGuide.rst 63b8f9b > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java > 1598f26 > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java > 463c9c3 > > flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java > c835172 > > Diff: https://reviews.apache.org/r/8864/diff/ > > > Testing > ------- > > All current tests pass. Added test to make sure new code works. > > > Thanks, > > Hari Shreedharan > >
