You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Juhani Connolly <ju...@gmail.com> on 2012/03/02 08:51:49 UTC
Review Request: Flume-1001 Support custom processors
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4152/
-----------------------------------------------------------
Review request for Flume.
Summary
-------
Made custom processors possible in the same way as sink and source: the type can represent a classname or a shorthand name from the typedef.
This addresses bug FLUME-1001.
https://issues.apache.org/jira/browse/FLUME-1001
Diffs
-----
flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java be1891b
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e
flume-ng-core/src/test/java/org/apache/flume/sink/TestSinkProcessorFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/4152/diff
Testing
-------
Existing tests pass, except I'm having an issue with TestNetcatSource which seems to be totally unrelated... It turns up on flume-728 head too, probably something in my environment
Added a new test to verify that processors created by shorthand name and by full class create the same class, verifying that creation by classname also works
Thanks,
Juhani
Re: Review Request: Flume-1001 Support custom processors
Posted by Juhani Connolly <ju...@gmail.com>.
> On 2012-03-02 16:47:32, Will McQueen wrote:
> > flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java, line 40
> > <https://reviews.apache.org/r/4152/diff/2/?file=87333#file87333line40>
> >
> > For consistency with the other types (SinkType, SourceType, ...), can we please put OTHER at the top of the enum listing?
> >
> > Also for same consistency, can we have the comment read:
> >
> > /**
> > * Place holder for custom sink processors not part of this enumeration.
> > */
Makes sense, done
> On 2012-03-02 16:47:32, Will McQueen wrote:
> > flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java, line 40
> > <https://reviews.apache.org/r/4152/diff/2/?file=87334#file87334line40>
> >
> > Before this line:
> > Preconditions.checkNotNull(context);
> > Preconditions.checkNotNull(sinks);
> > logger.debug("Creating instance of sink processor name {}, type {}", name, type);
Added precondition checks, changed the debug message a bit because processors belong to a sink group and thus themselves are not named.
> On 2012-03-02 16:47:32, Will McQueen wrote:
> > flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java, line 48
> > <https://reviews.apache.org/r/4152/diff/2/?file=87334#file87334line48>
> >
> > => "Sink processor type {} is a custom type"
> >
> > (this string output matches the convention used by DefaultSourceFactory)
ok
> On 2012-03-02 16:47:32, Will McQueen wrote:
> > flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java, line 66
> > <https://reviews.apache.org/r/4152/diff/2/?file=87334#file87334line66>
> >
> > add "sink", remove comma:
> >
> > =>"Unable to create sink processor type: "
> >
done
- Juhani
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4152/#review5557
-----------------------------------------------------------
On 2012-03-02 07:51:49, Juhani Connolly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4152/
> -----------------------------------------------------------
>
> (Updated 2012-03-02 07:51:49)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Made custom processors possible in the same way as sink and source: the type can represent a classname or a shorthand name from the typedef.
>
>
> This addresses bug FLUME-1001.
> https://issues.apache.org/jira/browse/FLUME-1001
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java be1891b
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e
> flume-ng-core/src/test/java/org/apache/flume/sink/TestSinkProcessorFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/4152/diff
>
>
> Testing
> -------
>
> Existing tests pass, except I'm having an issue with TestNetcatSource which seems to be totally unrelated... It turns up on flume-728 head too, probably something in my environment
>
> Added a new test to verify that processors created by shorthand name and by full class create the same class, verifying that creation by classname also works
>
>
> Thanks,
>
> Juhani
>
>
Re: Review Request: Flume-1001 Support custom processors
Posted by Will McQueen <wi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4152/#review5557
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java
<https://reviews.apache.org/r/4152/#comment12021>
For consistency with the other types (SinkType, SourceType, ...), can we please put OTHER at the top of the enum listing?
Also for same consistency, can we have the comment read:
/**
* Place holder for custom sink processors not part of this enumeration.
*/
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java
<https://reviews.apache.org/r/4152/#comment12035>
Before this line:
Preconditions.checkNotNull(context);
Preconditions.checkNotNull(sinks);
logger.debug("Creating instance of sink processor name {}, type {}", name, type);
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java
<https://reviews.apache.org/r/4152/#comment12032>
=> "Sink processor type {} is a custom type"
(this string output matches the convention used by DefaultSourceFactory)
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java
<https://reviews.apache.org/r/4152/#comment12041>
add "sink", remove comma:
=>"Unable to create sink processor type: "
flume-ng-core/src/test/java/org/apache/flume/sink/TestSinkProcessorFactory.java
<https://reviews.apache.org/r/4152/#comment12028>
Thank you for this test. It looks like it confirms that the FQDN and the enum name of the sink processor each return return the same SinkProcessor instance. I see that the intent of using the FQDN is to test the "OTHER" functionality.
- Will
On 2012-03-02 07:51:49, Juhani Connolly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4152/
> -----------------------------------------------------------
>
> (Updated 2012-03-02 07:51:49)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Made custom processors possible in the same way as sink and source: the type can represent a classname or a shorthand name from the typedef.
>
>
> This addresses bug FLUME-1001.
> https://issues.apache.org/jira/browse/FLUME-1001
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java be1891b
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e
> flume-ng-core/src/test/java/org/apache/flume/sink/TestSinkProcessorFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/4152/diff
>
>
> Testing
> -------
>
> Existing tests pass, except I'm having an issue with TestNetcatSource which seems to be totally unrelated... It turns up on flume-728 head too, probably something in my environment
>
> Added a new test to verify that processors created by shorthand name and by full class create the same class, verifying that creation by classname also works
>
>
> Thanks,
>
> Juhani
>
>
Re: Review Request: Flume-1001 Support custom processors
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4152/#review7829
-----------------------------------------------------------
Ship it!
+1
- Arvind
On 2012-05-09 09:56:14, Juhani Connolly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4152/
> -----------------------------------------------------------
>
> (Updated 2012-05-09 09:56:14)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Made custom processors possible in the same way as sink and source: the type can represent a classname or a shorthand name from the typedef.
>
>
> This addresses bug FLUME-1001.
> https://issues.apache.org/jira/browse/FLUME-1001
>
>
> Diffs
> -----
>
> flume-ng-core/src/test/java/org/apache/flume/sink/SinkProcessorFactoryTest.java PRE-CREATION
> flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkProcessorType.java d397bd3
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 3ff6737
>
> Diff: https://reviews.apache.org/r/4152/diff
>
>
> Testing
> -------
>
> Existing tests pass, except I'm having an issue with TestNetcatSource which seems to be totally unrelated... It turns up on flume-728 head too, probably something in my environment
>
> Added a new test to verify that processors created by shorthand name and by full class create the same class, verifying that creation by classname also works
>
>
> Thanks,
>
> Juhani
>
>
Re: Review Request: Flume-1001 Support custom processors
Posted by Juhani Connolly <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4152/
-----------------------------------------------------------
(Updated 2012-05-09 09:56:14.961414)
Review request for Flume.
Changes
-------
Updated against trunk
Summary
-------
Made custom processors possible in the same way as sink and source: the type can represent a classname or a shorthand name from the typedef.
This addresses bug FLUME-1001.
https://issues.apache.org/jira/browse/FLUME-1001
Diffs (updated)
-----
flume-ng-core/src/test/java/org/apache/flume/sink/SinkProcessorFactoryTest.java PRE-CREATION
flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkProcessorType.java d397bd3
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 3ff6737
Diff: https://reviews.apache.org/r/4152/diff
Testing
-------
Existing tests pass, except I'm having an issue with TestNetcatSource which seems to be totally unrelated... It turns up on flume-728 head too, probably something in my environment
Added a new test to verify that processors created by shorthand name and by full class create the same class, verifying that creation by classname also works
Thanks,
Juhani
Re: Review Request: Flume-1001 Support custom processors
Posted by Juhani Connolly <ju...@gmail.com>.
> On 2012-04-05 03:18:45, Brock Noland wrote:
> > looks good to me, just one nit. After you address to the nit please attache the patch to the JIRA.
Done and updated the issue.
This review is against the flume-728 so can't update the review file here, but it's just one row
- Juhani
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4152/#review6704
-----------------------------------------------------------
On 2012-03-05 03:25:20, Juhani Connolly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4152/
> -----------------------------------------------------------
>
> (Updated 2012-03-05 03:25:20)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Made custom processors possible in the same way as sink and source: the type can represent a classname or a shorthand name from the typedef.
>
>
> This addresses bug FLUME-1001.
> https://issues.apache.org/jira/browse/FLUME-1001
>
>
> Diffs
> -----
>
> flume-ng-core/src/test/java/org/apache/flume/sink/TestSinkProcessorFactory.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java be1891b
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e
>
> Diff: https://reviews.apache.org/r/4152/diff
>
>
> Testing
> -------
>
> Existing tests pass, except I'm having an issue with TestNetcatSource which seems to be totally unrelated... It turns up on flume-728 head too, probably something in my environment
>
> Added a new test to verify that processors created by shorthand name and by full class create the same class, verifying that creation by classname also works
>
>
> Thanks,
>
> Juhani
>
>
Re: Review Request: Flume-1001 Support custom processors
Posted by Brock Noland <br...@cloudera.com>.
> On 2012-04-05 03:18:45, Brock Noland wrote:
> > looks good to me, just one nit. After you address to the nit please attache the patch to the JIRA.
>
> Juhani Connolly wrote:
> Done and updated the issue.
> This review is against the flume-728 so can't update the review file here, but it's just one row
Yep, I think we can just attach the patch to the JIRA since it's just a single line.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4152/#review6704
-----------------------------------------------------------
On 2012-03-05 03:25:20, Juhani Connolly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4152/
> -----------------------------------------------------------
>
> (Updated 2012-03-05 03:25:20)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Made custom processors possible in the same way as sink and source: the type can represent a classname or a shorthand name from the typedef.
>
>
> This addresses bug FLUME-1001.
> https://issues.apache.org/jira/browse/FLUME-1001
>
>
> Diffs
> -----
>
> flume-ng-core/src/test/java/org/apache/flume/sink/TestSinkProcessorFactory.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java be1891b
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e
>
> Diff: https://reviews.apache.org/r/4152/diff
>
>
> Testing
> -------
>
> Existing tests pass, except I'm having an issue with TestNetcatSource which seems to be totally unrelated... It turns up on flume-728 head too, probably something in my environment
>
> Added a new test to verify that processors created by shorthand name and by full class create the same class, verifying that creation by classname also works
>
>
> Thanks,
>
> Juhani
>
>
Re: Review Request: Flume-1001 Support custom processors
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4152/#review6704
-----------------------------------------------------------
Ship it!
looks good to me, just one nit. After you address to the nit please attache the patch to the JIRA.
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java
<https://reviews.apache.org/r/4152/#comment14508>
INFO is more appropriate, people freak out when they see WARN.
- Brock
On 2012-03-05 03:25:20, Juhani Connolly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4152/
> -----------------------------------------------------------
>
> (Updated 2012-03-05 03:25:20)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Made custom processors possible in the same way as sink and source: the type can represent a classname or a shorthand name from the typedef.
>
>
> This addresses bug FLUME-1001.
> https://issues.apache.org/jira/browse/FLUME-1001
>
>
> Diffs
> -----
>
> flume-ng-core/src/test/java/org/apache/flume/sink/TestSinkProcessorFactory.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java be1891b
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e
>
> Diff: https://reviews.apache.org/r/4152/diff
>
>
> Testing
> -------
>
> Existing tests pass, except I'm having an issue with TestNetcatSource which seems to be totally unrelated... It turns up on flume-728 head too, probably something in my environment
>
> Added a new test to verify that processors created by shorthand name and by full class create the same class, verifying that creation by classname also works
>
>
> Thanks,
>
> Juhani
>
>
Re: Review Request: Flume-1001 Support custom processors
Posted by Juhani Connolly <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4152/
-----------------------------------------------------------
(Updated 2012-03-05 03:25:20.490086)
Review request for Flume.
Changes
-------
Addressed the suggested changes
Summary
-------
Made custom processors possible in the same way as sink and source: the type can represent a classname or a shorthand name from the typedef.
This addresses bug FLUME-1001.
https://issues.apache.org/jira/browse/FLUME-1001
Diffs (updated)
-----
flume-ng-core/src/test/java/org/apache/flume/sink/TestSinkProcessorFactory.java PRE-CREATION
flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java be1891b
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e
Diff: https://reviews.apache.org/r/4152/diff
Testing
-------
Existing tests pass, except I'm having an issue with TestNetcatSource which seems to be totally unrelated... It turns up on flume-728 head too, probably something in my environment
Added a new test to verify that processors created by shorthand name and by full class create the same class, verifying that creation by classname also works
Thanks,
Juhani