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