-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/#review14163
-----------------------------------------------------------

flume-ng-core/src/main/java/org/apache/flume/source/BasicSourceSemantics.java
<https://reviews.apache.org/r/8369/#comment30272>

    This section can be removed as it will be handled via FLUME-1772.
- Brock Noland
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/

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

> 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

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

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

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

> 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

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

Hi,

No problem -- I will create a separate JIRA for the additions to core.

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

> 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
>
> Hari Shreedharan wrote:
>     Thanks for your understanding, Brock!

No worries! I created https://issues.apache.org/jira/browse/FLUME-1777 for this and posted the patch on RB.
- Brock
-----------------------------------------------------------
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

> 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
>
> Hari Shreedharan wrote:
>     Thanks for your understanding, Brock!
>
> Brock Noland wrote:
>     No worries! I created https://issues.apache.org/jira/browse/FLUME-1777 for this and posted the patch on RB.

Also, FWIW, I do not extend AbstractSource in that patch because that class should probably be deprecated.
- Brock
-----------------------------------------------------------
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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/
-----------------------------------------------------------

(Updated Dec. 12, 2012, 10:13 p.m.)
Review request for Flume.
Changes
-------

Latest patch which applies on top of the latest FLUME-1777.
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 (updated)
-----

  flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
  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 6b465b2

Diff: https://reviews.apache.org/r/8369/diff/
Testing
-------

The tests which were added, pass.
Thanks,

Brock Noland

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/#review14411
-----------------------------------------------------------

flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/DefaultJMSMessageConverter.java
<https://reviews.apache.org/r/8369/#comment30662>

    Should be ObjectMessage

flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/DefaultJMSMessageConverter.java
<https://reviews.apache.org/r/8369/#comment30663>

    I think we should make the charset configurable.

flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/DefaultJMSMessageConverter.java
<https://reviews.apache.org/r/8369/#comment30664>

    We should wrap the exception in a FlumeException and throw.

flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSMessageConsumer.java
<https://reviews.apache.org/r/8369/#comment30673>

    pollTime should be called pollTimeout - it wasn't evident what this is until I actually realized it was the timeout value.

flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSMessageConsumer.java
<https://reviews.apache.org/r/8369/#comment30671>

    Since messageSelector is Nullable, this can throw NPE. Should the annotation be @NotNull?

flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSMessageConsumerFactory.java
<https://reviews.apache.org/r/8369/#comment30676>

    pollTime -> pollTimeout
- Hari Shreedharan
On Dec. 12, 2012, 10:13 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8369/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 10:13 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-doc/sphinx/FlumeUserGuide.rst 265f546
>   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

> On Dec. 13, 2012, 1:23 a.m., Hari Shreedharan wrote:
> > flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSMessageConsumer.java, line 100
> > <https://reviews.apache.org/r/8369/diff/3/?file=237577#file237577line100>
> >
> >     Since messageSelector is Nullable, this can throw NPE. Should the annotation be @NotNull?

Good catch! It should not be Nullable...
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/#review14411
-----------------------------------------------------------
On Dec. 12, 2012, 10:13 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8369/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 10:13 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-doc/sphinx/FlumeUserGuide.rst 265f546
>   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 6b465b2
>
> Diff: https://reviews.apache.org/r/8369/diff/
>
>
> Testing
> -------
>
> The tests which were added, pass.
>
>
> Thanks,
>
> Brock Noland
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/
-----------------------------------------------------------

(Updated Dec. 13, 2012, 3:41 p.m.)
Review request for Flume.
Changes
-------

Hari,

Thank you for the review! I have updated the patch based on it.

Brock
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 (updated)
-----

  flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
  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 6b465b2

Diff: https://reviews.apache.org/r/8369/diff/
Testing
-------

The tests which were added, pass.
Thanks,

Brock Noland

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/
-----------------------------------------------------------

(Updated Dec. 13, 2012, 3:42 p.m.)
Review request for Flume.
Changes
-------

Update description as some impl was moved to FLUME-1777
Description (updated)
-------

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.
This addresses bug FLUME-924.
    https://issues.apache.org/jira/browse/FLUME-924
Diffs
-----

  flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
  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 6b465b2

Diff: https://reviews.apache.org/r/8369/diff/
Testing
-------

The tests which were added, pass.
Thanks,

Brock Noland

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/
-----------------------------------------------------------

(Updated Dec. 13, 2012, 11:47 p.m.)
Review request for Flume.
Changes
-------

Update based on review.
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.
This addresses bug FLUME-924.
    https://issues.apache.org/jira/browse/FLUME-924
Diffs (updated)
-----

  flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
  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 6b465b2

Diff: https://reviews.apache.org/r/8369/diff/
Testing
-------

The tests which were added, pass.
Thanks,

Brock Noland

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/
-----------------------------------------------------------

(Updated Dec. 14, 2012, 12:39 a.m.)
Review request for Flume.
Changes
-------

Update javadoc
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.
This addresses bug FLUME-924.
    https://issues.apache.org/jira/browse/FLUME-924
Diffs (updated)
-----

  flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
  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 6b465b2

Diff: https://reviews.apache.org/r/8369/diff/
Testing
-------

The tests which were added, pass.
Thanks,

Brock Noland

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/
-----------------------------------------------------------

(Updated Dec. 14, 2012, 2:30 p.m.)
Review request for Flume.
Changes
-------

Update patch:

1) marks activemq as provided
2) Slightly changes Converter interface to allow for multiple flume events from a single JMS message
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.
This addresses bug FLUME-924.
    https://issues.apache.org/jira/browse/FLUME-924
Diffs (updated)
-----

  flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
  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 6b465b2

Diff: https://reviews.apache.org/r/8369/diff/
Testing
-------

The tests which were added, pass.
Thanks,

Brock Noland

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/#review14475
-----------------------------------------------------------

Ship it!
Brock,

Thanks for the patch. Great work! Could you document the changes in this patch (that the Builder is not mandatory, but if there is no builder - the class should be configurable). Once that is documented, please attach the patch to the jira.

- Hari Shreedharan
On Dec. 13, 2012, 11:47 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8369/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2012, 11:47 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.
>
>
> This addresses bug FLUME-924.
>     https://issues.apache.org/jira/browse/FLUME-924
>
>
> Diffs
> -----
>
>   flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
>   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 6b465b2
>
> Diff: https://reviews.apache.org/r/8369/diff/
>
>
> Testing
> -------
>
> The tests which were added, pass.
>
>
> Thanks,
>
> Brock Noland
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/#review14462
-----------------------------------------------------------
Mostly looks good, except the one comment I have.
flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSMessageConverter.java
<https://reviews.apache.org/r/8369/#comment30812>

    I'd rather make this class configurable and just call configure on this. I don't see a reason for using a builder, since we are passing in the context anyway.

flume-ng-sources/flume-jms-source/src/main/java/org/apache/flume/source/jms/JMSMessageConverter.java
<https://reviews.apache.org/r/8369/#comment30813>

    I'd rather make this class configurable and just call configure on this. I don't see a reason for using a builder, since we are passing in the context anyway. I don't see any value adding a builder when we are still passing in the configuration properties in the same way.
- Hari Shreedharan
On Dec. 13, 2012, 3:42 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8369/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2012, 3:42 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.
>
>
> This addresses bug FLUME-924.
>     https://issues.apache.org/jira/browse/FLUME-924
>
>
> Diffs
> -----
>
>   flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
>   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 6b465b2
>
> Diff: https://reviews.apache.org/r/8369/diff/
>
>
> Testing
> -------
>
> The tests which were added, pass.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 13, 2012, 8:58 p.m., Hari Shreedharan wrote:
> > Mostly looks good, except the one comment I have.
>
> Brock Noland wrote:
>     Hi,
>    
>     I'd like to keep the Builder interface simply because it's how the rest of the components work. Once we settle on a new Interface this one can be changed to align with them.
>    
>     Brock
>
> Hari Shreedharan wrote:
>     Which components? The HTTPSource does not - but others do. I don't see the point of a builder interface here - since it does not add any benefit (I don't see it in others also, but they are backwards incompatible to remove. I really really don't want to add something that is backwards incompatible, yet we do have a cleaner way of implementing it).
>
> Brock Noland wrote:
>     The other components I was referring to is Serializers, File Channel Encryption, and Interceptors. I do feel it is adding value in this case. By using this paradigm the members of the JMSMessageConverter implementors can be Immutable where as they cannot be via the Configurable interface.
>
> Hari Shreedharan wrote:
>     Brock,
>    
>     How about this? Make the Builder optional. Check if the builder exists - if yes, use it, else call newInstance(). Also the config should just have the class name of the converter and not require the Builder name (the $Builder stuff being required should not be the case).

I updated the patch, please see the JMSSource configure() method.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/#review14462
-----------------------------------------------------------
On Dec. 13, 2012, 11:47 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8369/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2012, 11:47 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.
>
>
> This addresses bug FLUME-924.
>     https://issues.apache.org/jira/browse/FLUME-924
>
>
> Diffs
> -----
>
>   flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
>   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

> On Dec. 13, 2012, 8:58 p.m., Hari Shreedharan wrote:
> > Mostly looks good, except the one comment I have.
>
> Brock Noland wrote:
>     Hi,
>    
>     I'd like to keep the Builder interface simply because it's how the rest of the components work. Once we settle on a new Interface this one can be changed to align with them.
>    
>     Brock
>
> Hari Shreedharan wrote:
>     Which components? The HTTPSource does not - but others do. I don't see the point of a builder interface here - since it does not add any benefit (I don't see it in others also, but they are backwards incompatible to remove. I really really don't want to add something that is backwards incompatible, yet we do have a cleaner way of implementing it).
>
> Brock Noland wrote:
>     The other components I was referring to is Serializers, File Channel Encryption, and Interceptors. I do feel it is adding value in this case. By using this paradigm the members of the JMSMessageConverter implementors can be Immutable where as they cannot be via the Configurable interface.

Brock,

How about this? Make the Builder optional. Check if the builder exists - if yes, use it, else call newInstance(). Also the config should just have the class name of the converter and not require the Builder name (the $Builder stuff being required should not be the case).
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/#review14462
-----------------------------------------------------------
On Dec. 13, 2012, 3:42 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8369/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2012, 3:42 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.
>
>
> This addresses bug FLUME-924.
>     https://issues.apache.org/jira/browse/FLUME-924
>
>
> Diffs
> -----
>
>   flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
>   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

> On Dec. 13, 2012, 8:58 p.m., Hari Shreedharan wrote:
> > Mostly looks good, except the one comment I have.
>
> Brock Noland wrote:
>     Hi,
>    
>     I'd like to keep the Builder interface simply because it's how the rest of the components work. Once we settle on a new Interface this one can be changed to align with them.
>    
>     Brock
>
> Hari Shreedharan wrote:
>     Which components? The HTTPSource does not - but others do. I don't see the point of a builder interface here - since it does not add any benefit (I don't see it in others also, but they are backwards incompatible to remove. I really really don't want to add something that is backwards incompatible, yet we do have a cleaner way of implementing it).

The other components I was referring to is Serializers, File Channel Encryption, and Interceptors. I do feel it is adding value in this case. By using this paradigm the members of the JMSMessageConverter implementors can be Immutable where as they cannot be via the Configurable interface.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/#review14462
-----------------------------------------------------------
On Dec. 13, 2012, 3:42 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8369/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2012, 3:42 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.
>
>
> This addresses bug FLUME-924.
>     https://issues.apache.org/jira/browse/FLUME-924
>
>
> Diffs
> -----
>
>   flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
>   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 6b465b2
>
> Diff: https://reviews.apache.org/r/8369/diff/
>
>
> Testing
> -------
>
> The tests which were added, pass.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 13, 2012, 8:58 p.m., Hari Shreedharan wrote:
> > Mostly looks good, except the one comment I have.
>
> Brock Noland wrote:
>     Hi,
>    
>     I'd like to keep the Builder interface simply because it's how the rest of the components work. Once we settle on a new Interface this one can be changed to align with them.
>    
>     Brock

Which components? The HTTPSource does not - but others do. I don't see the point of a builder interface here - since it does not add any benefit (I don't see it in others also, but they are backwards incompatible to remove. I really really don't want to add something that is backwards incompatible, yet we do have a cleaner way of implementing it).
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/#review14462
-----------------------------------------------------------
On Dec. 13, 2012, 3:42 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8369/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2012, 3:42 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.
>
>
> This addresses bug FLUME-924.
>     https://issues.apache.org/jira/browse/FLUME-924
>
>
> Diffs
> -----
>
>   flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
>   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 6b465b2
>
> Diff: https://reviews.apache.org/r/8369/diff/
>
>
> Testing
> -------
>
> The tests which were added, pass.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 13, 2012, 8:58 p.m., Hari Shreedharan wrote:
> > Mostly looks good, except the one comment I have.

Hi,

I'd like to keep the Builder interface simply because it's how the rest of the components work. Once we settle on a new Interface this one can be changed to align with them.

Brock
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8369/#review14462
-----------------------------------------------------------
On Dec. 13, 2012, 3:42 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8369/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2012, 3:42 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.
>
>
> This addresses bug FLUME-924.
>     https://issues.apache.org/jira/browse/FLUME-924
>
>
> Diffs
> -----
>
>   flume-ng-doc/sphinx/FlumeUserGuide.rst 265f546
>   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 6b465b2
>
> Diff: https://reviews.apache.org/r/8369/diff/
>
>
> Testing
> -------
>
> The tests which were added, pass.
>
>
> Thanks,
>
> Brock Noland
>
>