----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8369/#review14372 -----------------------------------------------------------
Brock, Thanks for the patch. The JMS Source code looks good generally. Generally, unless an interface change is required, we should not be changing stuff in the core module. I think adding the BasicSourceSemantics etc should not be required right? Shouldn't implementing the JMS Source as a PollableSource be good enough? I am ok with JMS stuff, but I think we should not change the core packages without discussion on a jira. I'd suggest changing that for now and pushing the JMS Source in as a PollableSource and then discussing this in a separate topic. Also, the risk factor of a new component being added as a separate is considerably less than changing anything in the core code - which affects every component. - Hari Shreedharan On Dec. 6, 2012, 6:34 p.m., Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8369/ > ----------------------------------------------------------- > > (Updated Dec. 6, 2012, 6:34 p.m.) > > > Review request for Flume. > > > Description > ------- > > Implements FLUME-924 (JMS Source). Some notes: > > 1) Should work for any JMS provider but only tested with ActiveMQ. > 2) Has unit tests as well as an ActiveMQ integration test > 3) Allows for a pluggable class to convert a JMS message to a Flume Event. > With that said, it provides a sensible default implementation. > 4) Adds a few abstract classes to core. The purpose of these was to clarify > the semantics of the configure, start, stop methods. > 5) Adds one protected method to AbtractSource to allow subclasses to set the > state. This is used to set the error state when an exception is thrown during > start, configure, or stop. > > > This addresses bug FLUME-924. > https://issues.apache.org/jira/browse/FLUME-924 > > > 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/AbstractSource.java > 0855de3 > > 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 > flume-ng-doc/sphinx/FlumeUserGuide.rst f8528bb > flume-ng-sources/flume-jms-source/pom.xml PRE-CREATION > > flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/DefaultJMSMessageConverter.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/InitialContextFactory.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSDestinationType.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSMessageConsumer.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSMessageConsumerFactory.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSMessageConverter.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSSource.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSSourceConfiguration.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/test/java/org/apache/flume/source/jms/JMSMessageConsumerTestBase.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/test/java/org/apache/flume/source/jms/TestDefaultJMSMessageConverter.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/test/java/org/apache/flume/source/jms/TestIntegrationActiveMQ.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/test/java/org/apache/flume/source/jms/TestJMSMessageConsumer.java > PRE-CREATION > > flume-ng-sources/flume-jms-source/src/test/java/org/apache/flume/source/jms/TestJMSSource.java > PRE-CREATION > flume-ng-sources/pom.xml 48f751d > pom.xml 53ac96b > > Diff: https://reviews.apache.org/r/8369/diff/ > > > Testing > ------- > > The tests which were added, pass. > > > Thanks, > > Brock Noland > >
