----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3088/#review5458 -----------------------------------------------------------
Hi, in general it looks good! Please remove the extra spaces, they show up in red. Some large changes have been merged in. Rebasing on trunk might be worthwhile. /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java <https://reviews.apache.org/r/3088/#comment11857> People often assume getters are very light weight and call them more often required. Should we store the value as an member variable as well as in the map as opposed to parsing the string each time? /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/formatter/output/BucketPath.java <https://reviews.apache.org/r/3088/#comment11861> Original code uses "" + but you use Long.toString below. Should we change it to the more proper Long.toString? /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/event/TestSimpleEvent.java <https://reviews.apache.org/r/3088/#comment11860> Most of the code does not use static imports. I am not used to this myself, but it's good to be consistent. - Brock On 2011-12-09 04:21:34, Vibul Imtarnasan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3088/ > ----------------------------------------------------------- > > (Updated 2011-12-09 04:21:34) > > > Review request for Flume. > > > Summary > ------- > > FLUME-872 > > > Diffs > ----- > > > /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSTextFormatter.java > 1212231 > > /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSWritableFormatter.java > 1212231 > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/event/EventBuilder.java > 1212231 > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java > 1212231 > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/formatter/output/BucketPath.java > 1212231 > > /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/event/TestSimpleEvent.java > PRE-CREATION > > /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java > 1212231 > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Event.java > 1212231 > > Diff: https://reviews.apache.org/r/3088/diff > > > Testing > ------- > > Unit test cases included > > > Thanks, > > Vibul > >
