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