[
https://issues.apache.org/jira/browse/FLUME-923?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13259746#comment-13259746
]
[email protected] commented on FLUME-923:
-----------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3723/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-04-20 19:02:01)
bq.
bq.
bq. Review request for Flume.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Here is a review form a rebased patch of FLUME-923.
bq. Sorry Dave, reviewboard didn't want your patch. So I re-exported from my
branch
bq.
bq.
bq. This addresses bug FLUME-923.
bq. https://issues.apache.org/jira/browse/FLUME-923
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. flume-ng-sinks/pom.xml acb3087
bq. pom.xml ed8092d
bq.
flume-ng-sinks/flume-jms-sink/src/test/java/org/apache/flume/sink/jms/TestJMSSink.java
PRE-CREATION
bq. flume-ng-node/pom.xml da0d15e
bq. flume-ng-sinks/flume-jms-sink/pom.xml PRE-CREATION
bq.
flume-ng-sinks/flume-jms-sink/src/main/java/org/apache/flume/sink/jms/JMSSink.java
PRE-CREATION
bq. flume-ng-core/src/main/java/org/apache/flume/sink/SinkType.java 6b08c09
bq. flume-ng-dist/pom.xml 642e681
bq.
bq. Diff: https://reviews.apache.org/r/3723/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Bruno
bq.
bq.
> Implement a JMS sink for Flume NG
> ---------------------------------
>
> Key: FLUME-923
> URL: https://issues.apache.org/jira/browse/FLUME-923
> Project: Flume
> Issue Type: New Feature
> Affects Versions: v1.0.0
> Reporter: Bruno Mahé
> Assignee: Bruno Mahé
> Attachments: FLUME-923-01-13-2.patch, FLUME-923-2.patch,
> FLUME-923-3.patch, FLUME-923-review-2.patch, FLUME-923-review.patch,
> FLUME-923.1.1.0-SNAPSHOT.patch, FLUME-923.patch, jms.conf
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
