----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8968/#review15412 -----------------------------------------------------------
Hari, the patch looks good! A few comments below. flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8968/#comment33207> nit: Is there a better variable name? Perhaps sinkCallbackPool? flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8968/#comment33199> nit: variable should be named isDelayTesting or isTimeoutTesting flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8968/#comment33201> Just curious as to why this was added or perhaps why it was missing? I am not too familiar with the async hbase api. flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8968/#comment33203> Curious why we converted to nano seconds from milleseconds? IMHO ms are easier to comprehend flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8968/#comment33214> It looks like it's possible for timeRemaining to be negative. That is if the condition returned with 1 nanosecond left and then it took longer than to to check the callbacks. Condition.await() java doc doesn't say what occurs with a negative number. flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8968/#comment33196> How come we are converting back and forth between nanos and millis? flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8968/#comment33195> nit: Should catch InterruptedException only flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8968/#comment33193> Same as above flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java <https://reviews.apache.org/r/8968/#comment33194> nit: Should catch InterruptedException only - Brock Noland On Jan. 16, 2013, 8:47 p.m., Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8968/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2013, 8:47 p.m.) > > > Review request for Flume. > > > Description > ------- > > Made changes to calculate remaining time and wait only for that long > > > This addresses bug FLUME-1842. > https://issues.apache.org/jira/browse/FLUME-1842 > > > Diffs > ----- > > flume-ng-doc/sphinx/FlumeUserGuide.rst 58a115e > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java > 6b34873 > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java > fad026c > > flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java > 1f61406 > > Diff: https://reviews.apache.org/r/8968/diff/ > > > Testing > ------- > > Added unit test. Current tests pass > > > Thanks, > > Hari Shreedharan > >
