----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3968/#review5427 -----------------------------------------------------------
Thanks for the patch Brock. Changes look good. A couple of suggestions: flume-ng-core/src/main/java/org/apache/flume/Context.java <https://reviews.apache.org/r/3968/#comment11830> Another thing to consider while you are at it is to provide a constructor that takes in a Map<String, String>. That way the two step call: Context c = new Context(); c.putAll(config); Can be collapsed into one: Context c = new Context(config); If you decide to do this, I suggest making a copy of the map that is passed in to insulate it from accidental external modification. flume-ng-core/src/main/java/org/apache/flume/Context.java <https://reviews.apache.org/r/3968/#comment11829> Since the parameters member variable is used for synchronization, you cannot replace it's reference. Replacing it's reference will break the thread safety of this class. Alternatively, you can instantiate parameters at the point of declaration or in constructor, and the clear() could then be applied to it directly. - Arvind On 2012-02-28 16:49:24, Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3968/ > ----------------------------------------------------------- > > (Updated 2012-02-28 16:49:24) > > > Review request for Flume. > > > Summary > ------- > > 1) Changing backing structure of Context from object to String > 2) Add getBoolean, getInteger, getLong to Context > 3) Remove get(key, clazz) > 4) Add javadoc to public methods of Context > 5) Change backing map of Context to synchronized map since it seems this > class could be accessed by multiple threads. > > > This addresses bug FLUME-978. > https://issues.apache.org/jira/browse/FLUME-978 > > > Diffs > ----- > > > flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/impl/JdbcChannelProviderImpl.java > 1cf1c0c > flume-ng-core/src/main/java/org/apache/flume/Context.java f1c8f85 > > flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java > a7d5f94 > flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java > e79490e > > flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java > c63d0a1 > > flume-ng-core/src/main/java/org/apache/flume/channel/PseudoTxnMemoryChannel.java > 1df580e > flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5440631 > flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java > 45c031d > flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 1adc5ff > flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java > 859f4fd > flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java 7b079f9 > flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java a96016c > flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java > d205bbc > flume-ng-core/src/test/java/org/apache/flume/TestContext.java a5e6aa8 > flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java > 3392dff > > flume-ng-core/src/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java > 93ad3bf > > flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java > b1e67f7 > > flume-ng-legacy-sources/flume-thrift-source/src/main/java/org/apache/flume/source/thriftLegacy/ThriftLegacySource.java > 5fe270a > > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java > 3da90a5 > > flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java > 0a5498f > > Diff: https://reviews.apache.org/r/3968/diff > > > Testing > ------- > > Unit tests added for all new methods and some exiting methods. > > All unit tests pass. > > > Thanks, > > Brock > >
