Flume
  1. Flume
  2. FLUME-949

Collapse PollableSink into Sink interface.

    Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: v1.0.0
    • Fix Version/s: v1.1.0
    • Component/s: None
    • Labels:
      None

      Description

      See the discussion thread:
      http://markmail.org/thread/e5rfcm2mwg54ofjo

      Summary: We do not have any non-pollable sink types and in order to keep the infrastructure and implementation simple, we want to standardize on the pollable sink model. Hence it makes sense to remove the explicit notion of PollableSink and move the process() method directly into the Sink definition.

      1. FLUME-949.patch
        12 kB
        Jarek Jarcec Cecho
      2. FLUME-949.patch
        21 kB
        Jarek Jarcec Cecho

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in flume-728 #103 (See https://builds.apache.org/job/flume-728/103/)
          FLUME-949. Collapse PollableSink into Sink interface.

          (Jarcec Checho via Arvind Prabhakar)

          arvind : http://svn.apache.org/viewvc/?view=rev&rev=1240313
          Files :

          • /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/PollableSink.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Sink.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/NullSink.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/PollableSinkRunner.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java
          • /incubator/flume/branches/flume-728/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java
          • /incubator/flume/branches/flume-728/flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java
          • /incubator/flume/branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java
          • /incubator/flume/branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java
          • /incubator/flume/branches/flume-728/flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java
          Show
          Hudson added a comment - Integrated in flume-728 #103 (See https://builds.apache.org/job/flume-728/103/ ) FLUME-949 . Collapse PollableSink into Sink interface. (Jarcec Checho via Arvind Prabhakar) arvind : http://svn.apache.org/viewvc/?view=rev&rev=1240313 Files : /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/PollableSink.java /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Sink.java /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/NullSink.java /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/PollableSinkRunner.java /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java /incubator/flume/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java /incubator/flume/branches/flume-728/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java /incubator/flume/branches/flume-728/flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java /incubator/flume/branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java /incubator/flume/branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java /incubator/flume/branches/flume-728/flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java
          Hide
          Arvind Prabhakar added a comment -

          Patch committed to flume-728 branch. Thanks Jarcec!

          Show
          Arvind Prabhakar added a comment - Patch committed to flume-728 branch. Thanks Jarcec!
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-02-03 18:14:53, Arvind Prabhakar wrote:

          > +1. Please attach the patch to the JIRA.

          >

          > Thanks

          I've just uploaded latest version of the patch to JIRA. Sorry that I did not do it earlier, I just needed to get out of the bus

          • Jarek

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3739/#review4808
          -----------------------------------------------------------

          On 2012-02-03 17:21:32, Jarek Cecho wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3739/

          -----------------------------------------------------------

          (Updated 2012-02-03 17:21:32)

          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 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Sink.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/NullSink.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/PollableSinkRunner.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 1240228

          /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 1240228

          /branches/flume-728/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1240228

          /branches/flume-728/flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java 1240228

          /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 1240228

          /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java 1240228

          /branches/flume-728/flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 1240228

          Diff: https://reviews.apache.org/r/3739/diff

          Testing

          -------

          mvn test passes correctly for entire flume-ng project

          Thanks,

          Jarek

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-02-03 18:14:53, Arvind Prabhakar wrote: > +1. Please attach the patch to the JIRA. > > Thanks I've just uploaded latest version of the patch to JIRA. Sorry that I did not do it earlier, I just needed to get out of the bus Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3739/#review4808 ----------------------------------------------------------- On 2012-02-03 17:21:32, Jarek Cecho wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3739/ ----------------------------------------------------------- (Updated 2012-02-03 17:21:32) 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 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Sink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/NullSink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/PollableSinkRunner.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 1240228 /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 1240228 /branches/flume-728/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1240228 /branches/flume-728/flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java 1240228 /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 1240228 /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java 1240228 /branches/flume-728/flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 1240228 Diff: https://reviews.apache.org/r/3739/diff Testing ------- mvn test passes correctly for entire flume-ng project Thanks, Jarek
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3739/#review4808
          -----------------------------------------------------------

          Ship it!

          +1. Please attach the patch to the JIRA.

          Thanks

          • Arvind

          On 2012-02-03 17:21:32, Jarek Cecho wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3739/

          -----------------------------------------------------------

          (Updated 2012-02-03 17:21:32)

          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 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Sink.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/NullSink.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/PollableSinkRunner.java 1240228

          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 1240228

          /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 1240228

          /branches/flume-728/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1240228

          /branches/flume-728/flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java 1240228

          /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 1240228

          /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java 1240228

          /branches/flume-728/flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 1240228

          Diff: https://reviews.apache.org/r/3739/diff

          Testing

          -------

          mvn test passes correctly for entire flume-ng project

          Thanks,

          Jarek

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3739/#review4808 ----------------------------------------------------------- Ship it! +1. Please attach the patch to the JIRA. Thanks Arvind On 2012-02-03 17:21:32, Jarek Cecho wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3739/ ----------------------------------------------------------- (Updated 2012-02-03 17:21:32) 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 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Sink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/NullSink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/PollableSinkRunner.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 1240228 /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 1240228 /branches/flume-728/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1240228 /branches/flume-728/flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java 1240228 /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 1240228 /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java 1240228 /branches/flume-728/flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 1240228 Diff: https://reviews.apache.org/r/3739/diff Testing ------- mvn test passes correctly for entire flume-ng project Thanks, Jarek
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3739/
          -----------------------------------------------------------

          (Updated 2012-02-03 17:21:32.987357)

          Review request for Flume and Arvind Prabhakar.

          Changes
          -------

          I've applied both suggestions:

          1) I've collapsed PollableSinkRunner into SinkRunner class

          2) I've removed "implements Sink" when we were subclassing AbstractSink (sorry that I've missed that earlier)

          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 (updated)


          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/PollableSink.java 1240228
          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Sink.java 1240228
          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 1240228
          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 1240228
          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java 1240228
          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/NullSink.java 1240228
          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/PollableSinkRunner.java 1240228
          /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 1240228
          /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 1240228
          /branches/flume-728/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1240228
          /branches/flume-728/flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java 1240228
          /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 1240228
          /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java 1240228
          /branches/flume-728/flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 1240228

          Diff: https://reviews.apache.org/r/3739/diff

          Testing
          -------

          mvn test passes correctly for entire flume-ng project

          Thanks,

          Jarek

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3739/ ----------------------------------------------------------- (Updated 2012-02-03 17:21:32.987357) Review request for Flume and Arvind Prabhakar. Changes ------- I've applied both suggestions: 1) I've collapsed PollableSinkRunner into SinkRunner class 2) I've removed "implements Sink" when we were subclassing AbstractSink (sorry that I've missed that earlier) 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 (updated) /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/PollableSink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Sink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/NullSink.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/PollableSinkRunner.java 1240228 /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 1240228 /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 1240228 /branches/flume-728/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1240228 /branches/flume-728/flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java 1240228 /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 1240228 /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java 1240228 /branches/flume-728/flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 1240228 Diff: https://reviews.apache.org/r/3739/diff Testing ------- mvn test passes correctly for entire flume-ng project Thanks, Jarek
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-02-03 15:18:30, Arvind Prabhakar wrote:

          > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java, line 25

          > <https://reviews.apache.org/r/3739/diff/1/?file=71899#file71899line25>

          >

          > 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.

          Thank you for your feedback sir. I thought so, but I wanted to check anyway. I'll try to put together patch updated as soon as possible.

          • Jarek

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3739/#review4802
          -----------------------------------------------------------

          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

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-02-03 15:18:30, Arvind Prabhakar wrote: > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java, line 25 > < https://reviews.apache.org/r/3739/diff/1/?file=71899#file71899line25 > > > 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. Thank you for your feedback sir. I thought so, but I wanted to check anyway. I'll try to put together patch updated as soon as possible. Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3739/#review4802 ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3739/
          -----------------------------------------------------------

          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3739/ ----------------------------------------------------------- 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

            People

            • Assignee:
              Jarek Jarcec Cecho
              Reporter:
              Arvind Prabhakar
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development