Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Flume, mail # dev - Review Request: FLUME-1777: AbstractSource does not provide enough implementation for sub-classes
Brock Noland
2012-12-12, 21:17
Hari Shreedharan
2012-12-13, 00:13
Brock Noland
2012-12-13, 14:58
Hari Shreedharan
2012-12-13, 19:45
Copy link to this message
-
Re: Review Request: FLUME-1777: AbstractSource does not provide enough implementation for sub-classes
Brock Noland 2012-12-13, 14:56


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