You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Brock Noland <br...@cloudera.com> on 2012/12/12 22:17:42 UTC
Review Request: FLUME-1777: AbstractSource does not provide enough
implementation for sub-classes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8558/
-----------------------------------------------------------
Review request for Flume.
Description
-------
Adds BasicSourceSemantics (named similar to the Channel abstract classes) which sets the life cycle state appropriate, doesn't require the use of super, and allows subclasses to set the lifecycle state. Additionall adds AbstractPollableSource and AbstractEventDrivenSource.
This addresses bug FLUME-1777.
https://issues.apache.org/jira/browse/FLUME-1777
Diffs
-----
flume-ng-core/src/main/java/org/apache/flume/source/AbstractEventDrivenSource.java PRE-CREATION
flume-ng-core/src/main/java/org/apache/flume/source/AbstractPollableSource.java PRE-CREATION
flume-ng-core/src/main/java/org/apache/flume/source/BasicSourceSemantics.java PRE-CREATION
flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java d4d818a
flume-ng-core/src/test/java/org/apache/flume/source/TestAbstractPollableSource.java PRE-CREATION
flume-ng-core/src/test/java/org/apache/flume/source/TestBasicSourceSemantics.java PRE-CREATION
Diff: https://reviews.apache.org/r/8558/diff/
Testing
-------
Unit tests added, pass.
Thanks,
Brock Noland
Re: Review Request: FLUME-1777: AbstractSource does not provide enough
implementation for sub-classes
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8558/#review14459
-----------------------------------------------------------
Ship it!
Ship It!
- Hari Shreedharan
On Dec. 13, 2012, 2:58 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8558/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2012, 2:58 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Adds BasicSourceSemantics (named similar to the Channel abstract classes) which sets the life cycle state appropriate, doesn't require the use of super, and allows subclasses to set the lifecycle state. Additionall adds AbstractPollableSource and AbstractEventDrivenSource.
>
>
> This addresses bug FLUME-1777.
> https://issues.apache.org/jira/browse/FLUME-1777
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/AbstractEventDrivenSource.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/source/AbstractPollableSource.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/source/BasicSourceSemantics.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java d4d818a
> flume-ng-core/src/test/java/org/apache/flume/source/TestAbstractPollableSource.java PRE-CREATION
> flume-ng-core/src/test/java/org/apache/flume/source/TestBasicSourceSemantics.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8558/diff/
>
>
> Testing
> -------
>
> Unit tests added, pass.
>
>
> Thanks,
>
> Brock Noland
>
>
Re: Review Request: FLUME-1777: AbstractSource does not provide enough
implementation for sub-classes
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8558/
-----------------------------------------------------------
(Updated Dec. 13, 2012, 2:58 p.m.)
Review request for Flume.
Changes
-------
Updated based on hari's review.
Description
-------
Adds BasicSourceSemantics (named similar to the Channel abstract classes) which sets the life cycle state appropriate, doesn't require the use of super, and allows subclasses to set the lifecycle state. Additionall adds AbstractPollableSource and AbstractEventDrivenSource.
This addresses bug FLUME-1777.
https://issues.apache.org/jira/browse/FLUME-1777
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/AbstractEventDrivenSource.java PRE-CREATION
flume-ng-core/src/main/java/org/apache/flume/source/AbstractPollableSource.java PRE-CREATION
flume-ng-core/src/main/java/org/apache/flume/source/BasicSourceSemantics.java PRE-CREATION
flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java d4d818a
flume-ng-core/src/test/java/org/apache/flume/source/TestAbstractPollableSource.java PRE-CREATION
flume-ng-core/src/test/java/org/apache/flume/source/TestBasicSourceSemantics.java PRE-CREATION
Diff: https://reviews.apache.org/r/8558/diff/
Testing
-------
Unit tests added, pass.
Thanks,
Brock Noland
Re: Review Request: FLUME-1777: AbstractSource does not provide enough
implementation for sub-classes
Posted by Brock Noland <br...@cloudera.com>.
> On Dec. 13, 2012, 12:13 a.m., Hari Shreedharan wrote:
> > flume-ng-core/src/main/java/org/apache/flume/source/BasicSourceSemantics.java, line 44
> > <https://reviews.apache.org/r/8558/diff/2/?file=237514#file237514line44>
> >
> > Looks like this class implements all the methods from AbstractSource. Is there a reason you chose not to inherit from the same class?
I'd like to see AbstractSource deprecated. It for example requires the use of super when overriding methods such as start(), and stop(). This is a a poor practice and I think it's better to provide explicit hooks which is what BasicSourceSemantics does.
> On Dec. 13, 2012, 12:13 a.m., Hari Shreedharan wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestAbstractPollableSource.java, line 70
> > <https://reviews.apache.org/r/8558/diff/2/?file=237516#file237516line70>
> >
> > I prefer using @Test (expected = FlumeException.class). It is often cleaner
Yeah I think this was a copy paste error. I like expected = as well except in the case where a prerequisite method could throw the same exception. That is not the case here.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8558/#review14408
-----------------------------------------------------------
On Dec. 12, 2012, 9:17 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8558/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 9:17 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Adds BasicSourceSemantics (named similar to the Channel abstract classes) which sets the life cycle state appropriate, doesn't require the use of super, and allows subclasses to set the lifecycle state. Additionall adds AbstractPollableSource and AbstractEventDrivenSource.
>
>
> This addresses bug FLUME-1777.
> https://issues.apache.org/jira/browse/FLUME-1777
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/AbstractEventDrivenSource.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/source/AbstractPollableSource.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/source/BasicSourceSemantics.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java d4d818a
> flume-ng-core/src/test/java/org/apache/flume/source/TestAbstractPollableSource.java PRE-CREATION
> flume-ng-core/src/test/java/org/apache/flume/source/TestBasicSourceSemantics.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8558/diff/
>
>
> Testing
> -------
>
> Unit tests added, pass.
>
>
> Thanks,
>
> Brock Noland
>
>
Re: Review Request: FLUME-1777: AbstractSource does not provide enough
implementation for sub-classes
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8558/#review14408
-----------------------------------------------------------
Brock,
Generally looks excellent. Some relatively minor comments are below. Also it does not look like the spy is actually being used in the tests. In that case you might not need to use spy at all. Using the dummy classes directly would work.
flume-ng-core/src/main/java/org/apache/flume/source/BasicSourceSemantics.java
<https://reviews.apache.org/r/8558/#comment30645>
Looks like this class implements all the methods from AbstractSource. Is there a reason you chose not to inherit from the same class?
flume-ng-core/src/main/java/org/apache/flume/source/BasicSourceSemantics.java
<https://reviews.apache.org/r/8558/#comment30646>
Do we really need these methods, and force implementation to implement this? AFAICT, it looks like these methods will simply log the error? Why not just implement it here? The name of the component can be prepended to the log message to make the logging clearer
flume-ng-core/src/test/java/org/apache/flume/source/TestAbstractPollableSource.java
<https://reviews.apache.org/r/8558/#comment30647>
I prefer using @Test (expected = FlumeException.class). It is often cleaner
flume-ng-core/src/test/java/org/apache/flume/source/TestAbstractPollableSource.java
<https://reviews.apache.org/r/8558/#comment30655>
Same as above. Consider using @Test (expected= .. )
- Hari Shreedharan
On Dec. 12, 2012, 9:17 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8558/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 9:17 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Adds BasicSourceSemantics (named similar to the Channel abstract classes) which sets the life cycle state appropriate, doesn't require the use of super, and allows subclasses to set the lifecycle state. Additionall adds AbstractPollableSource and AbstractEventDrivenSource.
>
>
> This addresses bug FLUME-1777.
> https://issues.apache.org/jira/browse/FLUME-1777
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/AbstractEventDrivenSource.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/source/AbstractPollableSource.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/source/BasicSourceSemantics.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java d4d818a
> flume-ng-core/src/test/java/org/apache/flume/source/TestAbstractPollableSource.java PRE-CREATION
> flume-ng-core/src/test/java/org/apache/flume/source/TestBasicSourceSemantics.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8558/diff/
>
>
> Testing
> -------
>
> Unit tests added, pass.
>
>
> Thanks,
>
> Brock Noland
>
>