You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Jarek Cecho <ja...@apache.org> on 2012/02/02 23:10:16 UTC
Review Request: FLUME-949: Collapse PollableSink into Sink interface.
-----------------------------------------------------------
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
Re: Review Request: FLUME-949: Collapse PollableSink into Sink interface.
Posted by Jarek Cecho <ja...@apache.org>.
> 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
>
>
Re: Review Request: FLUME-949: Collapse PollableSink into Sink interface.
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: FLUME-949: Collapse PollableSink into Sink interface.
Posted by Jarek Cecho <ja...@apache.org>.
> 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
>
>
Re: Review Request: FLUME-949: Collapse PollableSink into Sink interface.
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: FLUME-949: Collapse PollableSink into Sink interface.
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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