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