> On Dec. 12, 2012, 8:21 p.m., Hari Shreedharan wrote: > > 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. > > Brock Noland wrote: > Hi, > > Thank you for the review Hari! One method is added to AbstractSource but > other than that, nothing in this patch affects core. That is all the existing > components extends AbstractSource and could, in other changes, extend > BasicSourceSemantics (or more likely > AbstractPollableSoure/AbstractEventDrivenSource) if we found it useful. As > such, I don't see how adding three classes to core is risky? > > Brock > > Hari Shreedharan wrote: > Hi Brock, > > What I meant was that when we add a new submodule - we usually just add > that..not change any code in the core. For one it is easier to track changes > (using git history) + usually a discussion on the jira is beneficial. If we > want to add the BasicSourceSemantics etc., we should do it on a different > jira - so we can track why we made the changes. > > On a related note, I think we already have too much "hierarchy" in the > Basic*Semantics stuff. I'd rather merge the Sematics code into the Abstract* > code. I don't see a need for BasicSinkSemantics if that code is simply in > AbstractSink. > > Brock Noland wrote: > Hi, > > No problem -- I will create a separate JIRA for the additions to core. > > Brock
Thanks for your understanding, Brock! - Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8369/#review14372 ----------------------------------------------------------- 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 > >
