You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Bruno Mahé <br...@cloudera.com> on 2012/02/01 20:02:08 UTC
Review Request: Implement a JMS sink for Flume NG
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3723/
-----------------------------------------------------------
Review request for Flume.
Summary
-------
Here is a review form a rebased patch of FLUME-923.
Sorry Dave, reviewboard didn't want your patch. So I re-exported from my branch
This addresses bug FLUME-923.
https://issues.apache.org/jira/browse/FLUME-923
Diffs
-----
flume-ng-core/src/main/java/org/apache/flume/sink/SinkType.java 6b08c09
flume-ng-dist/pom.xml 19f6b0c
flume-ng-node/pom.xml b9b062e
flume-ng-sinks/flume-jms-sink/pom.xml PRE-CREATION
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java PRE-CREATION
flume-ng-sinks/flume-jms-sink/src/test/java/org/apache/flume/sink/jms/TestJMSSink.java PRE-CREATION
flume-ng-sinks/pom.xml b6cf477
pom.xml abee9a5
Diff: https://reviews.apache.org/r/3723/diff
Testing
-------
Thanks,
Bruno
Re: Review Request: Implement a JMS sink for Flume NG
Posted by Bruno Mahé <br...@cloudera.com>.
> On 2012-02-01 20:48:46, Arvind Prabhakar wrote:
> > Thanks for the patch Bruno. The implementation needs to be slightly reworked to ensure thread-safe handling of jmsSession object. What I suggest is that you refactor the implementation so that:
> >
> > 1. configure() method creates the necessary naming context
> > 2. start() method initializes the connection factory and creates a connection.
> > 3. process() method creates a session, uses it to publish the event and then commits it.
> > 4. stop() method closes the connection.
> >
> > Another point of consideration is to enable batch message support in this. However, that can be done at a later stage via a separate issue.
> >
> > Thanks
Thanks a lot for the feedback!
I will rework it as advised and update the review
- Bruno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3723/#review4756
-----------------------------------------------------------
On 2012-02-01 19:02:07, Bruno Mahé wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3723/
> -----------------------------------------------------------
>
> (Updated 2012-02-01 19:02:07)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Here is a review form a rebased patch of FLUME-923.
> Sorry Dave, reviewboard didn't want your patch. So I re-exported from my branch
>
>
> This addresses bug FLUME-923.
> https://issues.apache.org/jira/browse/FLUME-923
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkType.java 6b08c09
> flume-ng-dist/pom.xml 19f6b0c
> flume-ng-node/pom.xml b9b062e
> flume-ng-sinks/flume-jms-sink/pom.xml PRE-CREATION
> flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java PRE-CREATION
> flume-ng-sinks/flume-jms-sink/src/test/java/org/apache/flume/sink/jms/TestJMSSink.java PRE-CREATION
> flume-ng-sinks/pom.xml b6cf477
> pom.xml abee9a5
>
> Diff: https://reviews.apache.org/r/3723/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruno
>
>
Re: Review Request: Implement a JMS sink for Flume NG
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3723/#review4756
-----------------------------------------------------------
Thanks for the patch Bruno. The implementation needs to be slightly reworked to ensure thread-safe handling of jmsSession object. What I suggest is that you refactor the implementation so that:
1. configure() method creates the necessary naming context
2. start() method initializes the connection factory and creates a connection.
3. process() method creates a session, uses it to publish the event and then commits it.
4. stop() method closes the connection.
Another point of consideration is to enable batch message support in this. However, that can be done at a later stage via a separate issue.
Thanks
- Arvind
On 2012-02-01 19:02:07, Bruno Mahé wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3723/
> -----------------------------------------------------------
>
> (Updated 2012-02-01 19:02:07)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Here is a review form a rebased patch of FLUME-923.
> Sorry Dave, reviewboard didn't want your patch. So I re-exported from my branch
>
>
> This addresses bug FLUME-923.
> https://issues.apache.org/jira/browse/FLUME-923
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkType.java 6b08c09
> flume-ng-dist/pom.xml 19f6b0c
> flume-ng-node/pom.xml b9b062e
> flume-ng-sinks/flume-jms-sink/pom.xml PRE-CREATION
> flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java PRE-CREATION
> flume-ng-sinks/flume-jms-sink/src/test/java/org/apache/flume/sink/jms/TestJMSSink.java PRE-CREATION
> flume-ng-sinks/pom.xml b6cf477
> pom.xml abee9a5
>
> Diff: https://reviews.apache.org/r/3723/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruno
>
>
Re: Review Request: Implement a JMS sink for Flume NG
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3723/#review7139
-----------------------------------------------------------
Thanks for the patch Bruno. Some comments follow:
* General comment: Looks like you have used tabs for indentation. The convention for Flume is to not use any tabs but instead use two spaces per indent. Please reformat your patch accordingly.
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java
<https://reviews.apache.org/r/3723/#comment15773>
Please remove this from class scope. Instead the jmsSession instance should be created at the point of use.
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java
<https://reviews.apache.org/r/3723/#comment15775>
Suggest renaming this method to initializeJmsResources()
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java
<https://reviews.apache.org/r/3723/#comment15774>
This should be removed.
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java
<https://reviews.apache.org/r/3723/#comment15777>
Suggest renaming this method to something like closeJmsResources()
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java
<https://reviews.apache.org/r/3723/#comment15776>
This should be removed.
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java
<https://reviews.apache.org/r/3723/#comment15771>
The jmsSession should not be a class member variable. Please make it local, create it within this block and commit it on success. Alternatively you could create it within the sendEvent method itself and commit it there.
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java
<https://reviews.apache.org/r/3723/#comment15770>
This should be within the else block.
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java
<https://reviews.apache.org/r/3723/#comment15772>
Since you are already catching the general Exception below, I suggest removing the catch ChannelException block altogether.
- Arvind
On 2012-04-20 19:02:01, Bruno Mahé wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3723/
> -----------------------------------------------------------
>
> (Updated 2012-04-20 19:02:01)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Here is a review form a rebased patch of FLUME-923.
> Sorry Dave, reviewboard didn't want your patch. So I re-exported from my branch
>
>
> This addresses bug FLUME-923.
> https://issues.apache.org/jira/browse/FLUME-923
>
>
> Diffs
> -----
>
> flume-ng-sinks/pom.xml acb3087
> pom.xml ed8092d
> flume-ng-sinks/flume-jms-sink/src/test/java/org/apache/flume/sink/jms/TestJMSSink.java PRE-CREATION
> flume-ng-node/pom.xml da0d15e
> flume-ng-sinks/flume-jms-sink/pom.xml PRE-CREATION
> flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkType.java 6b08c09
> flume-ng-dist/pom.xml 642e681
>
> Diff: https://reviews.apache.org/r/3723/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruno
>
>
Re: Review Request: Implement a JMS sink for Flume NG
Posted by Bruno Mahé <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3723/
-----------------------------------------------------------
(Updated 2012-04-20 19:02:01.333363)
Review request for Flume.
Summary
-------
Here is a review form a rebased patch of FLUME-923.
Sorry Dave, reviewboard didn't want your patch. So I re-exported from my branch
This addresses bug FLUME-923.
https://issues.apache.org/jira/browse/FLUME-923
Diffs
-----
flume-ng-sinks/pom.xml acb3087
pom.xml ed8092d
flume-ng-sinks/flume-jms-sink/src/test/java/org/apache/flume/sink/jms/TestJMSSink.java PRE-CREATION
flume-ng-node/pom.xml da0d15e
flume-ng-sinks/flume-jms-sink/pom.xml PRE-CREATION
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java PRE-CREATION
flume-ng-core/src/main/java/org/apache/flume/sink/SinkType.java 6b08c09
flume-ng-dist/pom.xml 642e681
Diff: https://reviews.apache.org/r/3723/diff
Testing
-------
Thanks,
Bruno
Re: Review Request: Implement a JMS sink for Flume NG
Posted by Bruno Mahé <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3723/
-----------------------------------------------------------
(Updated 2012-04-20 01:05:11.472519)
Review request for Flume.
Changes
-------
Here is an updated patch adressing Arvind's concerns.
Let me know if more work need to be done
Summary
-------
Here is a review form a rebased patch of FLUME-923.
Sorry Dave, reviewboard didn't want your patch. So I re-exported from my branch
This addresses bug FLUME-923.
https://issues.apache.org/jira/browse/FLUME-923
Diffs (updated)
-----
flume-ng-sinks/pom.xml acb3087
pom.xml ed8092d
flume-ng-sinks/flume-jms-sink/src/test/java/org/apache/flume/sink/jms/TestJMSSink.java PRE-CREATION
flume-ng-node/pom.xml da0d15e
flume-ng-sinks/flume-jms-sink/pom.xml PRE-CREATION
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java PRE-CREATION
flume-ng-core/src/main/java/org/apache/flume/sink/SinkType.java 6b08c09
flume-ng-dist/pom.xml 642e681
Diff: https://reviews.apache.org/r/3723/diff
Testing
-------
Thanks,
Bruno