----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5179/#review8018 -----------------------------------------------------------
Thanks for the patch, Mingjie! Couple of suggestions are inline. Also, please remove trailing whitespaces in the code, and tabs too. flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java <https://reviews.apache.org/r/5179/#comment17395> nit: Can you put some parantheses in these conditionals so that they are easier to read, something like: fileExtension = (extension == null ? extension : extension.trim()); There is a similar one in the unit tests also I believe. flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java <https://reviews.apache.org/r/5179/#comment17394> If the file extension is not specified in the configuration, it would be better to use the default extension for the codec, as was the behavior before. This is good for 2 reasons: 1) Older configuration files can be used without any change and it will still give the same behavior as before 2) It is always better to default to the default extension of the file type. - Hari On 2012-05-21 18:13:28, Mingjie Lai wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5179/ > ----------------------------------------------------------- > > (Updated 2012-05-21 18:13:28) > > > Review request for Flume. > > > Summary > ------- > > FLUME-1209: Support file name extension for HDFS sink. > > > Diffs > ----- > > > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java > 91cb822 > > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java > 1b61cad > > flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java > 0f78f37 > > Diff: https://reviews.apache.org/r/5179/diff > > > Testing > ------- > > Test case passed locally. > > > Thanks, > > Mingjie > >
