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

Switch to Plain View
Flume >> mail # dev >> Review Request: FLUME-924: Implement a JMS source for Flume NG


+
Brock Noland 2012-12-06, 18:34
+
Brock Noland 2012-12-07, 17:55
+
Hari Shreedharan 2012-12-12, 20:21
+
Brock Noland 2012-12-12, 20:27
+
Hari Shreedharan 2012-12-12, 20:41
+
Brock Noland 2012-12-12, 20:45
+
Hari Shreedharan 2012-12-12, 21:00
+
Brock Noland 2012-12-12, 21:26
Copy link to this message
-
Re: Review Request: FLUME-924: Implement a JMS source for Flume NG


> 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
+
Brock Noland 2012-12-12, 22:13
+
Hari Shreedharan 2012-12-13, 01:23
+
Brock Noland 2012-12-13, 15:35
+
Brock Noland 2012-12-13, 15:41
+
Brock Noland 2012-12-13, 15:42
+
Hari Shreedharan 2012-12-13, 20:58
+
Brock Noland 2012-12-13, 21:25
+
Hari Shreedharan 2012-12-13, 21:37
+
Brock Noland 2012-12-13, 21:46
+
Hari Shreedharan 2012-12-13, 22:01
+
Brock Noland 2012-12-13, 23:47
+
Brock Noland 2012-12-13, 23:47
+
Hari Shreedharan 2012-12-14, 00:00
+
Brock Noland 2012-12-14, 00:39
+
Brock Noland 2012-12-14, 14:30