----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3739/#review4802 -----------------------------------------------------------
Changes look good Jarcec. A couple of feedback points below. /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java <https://reviews.apache.org/r/3739/#comment10564> Since now we have only a single Sink type, there does not seem to be any value in having a SinkRunner interface and a default implementation. Instead, I suggest removing the SinkRunner interface and making the SinkRunner as a concrete public class. /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java <https://reviews.apache.org/r/3739/#comment10565> <nit: applies to other sinks as well>: Since AbstractSink already implements Sink, the specification implementing Sink is redundant. Suggest you modify this (and other) sink to just extend AbstractSink. - Arvind On 2012-02-02 22:10:16, Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3739/ > ----------------------------------------------------------- > > (Updated 2012-02-02 22:10:16) > > > Review request for Flume and Arvind Prabhakar. > > > Summary > ------- > > I've collapsed PollableSink into Sink interface and fixed all other classes. > I'm wondering whether it would make sense to also collapse PollableSinkRunner > into SinkRunner class? > > > This addresses bug FLUME-949. > https://issues.apache.org/jira/browse/FLUME-949 > > > Diffs > ----- > > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/PollableSink.java > 1239790 > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Sink.java > 1239790 > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java > 1239790 > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java > 1239790 > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java > 1239790 > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/NullSink.java > 1239790 > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/PollableSinkRunner.java > 1239790 > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java > 1239790 > > /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java > 1239790 > > /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java > 1239790 > > /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java > 1239790 > > /branches/flume-728/flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java > 1239790 > > Diff: https://reviews.apache.org/r/3739/diff > > > Testing > ------- > > mvn test passes correctly for entire flume-ng project > > > Thanks, > > Jarek > >
