----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16179/#review56298 -----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java <https://reviews.apache.org/r/16179/#comment96609> Please don't use asterisks import, enumerate what exactly do you need. flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java <https://reviews.apache.org/r/16179/#comment96611> I'm missing validation of inserted data. Shouldn't there be some assert statements? Also the test itself can be probably enhanced. What about having constant message (10 bytes) and changing the maxBytesToDump option from 1 to 20 to ensure that we get on the output expected outcome? flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java <https://reviews.apache.org/r/16179/#comment96610> I'm assuming that this main class should not be part of the final patch? - Jarek Cecho On Dec. 11, 2013, 9:58 a.m., Ashish Paliwal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16179/ > ----------------------------------------------------------- > > (Updated Dec. 11, 2013, 9:58 a.m.) > > > Review request for Flume. > > > Bugs: FLUME-2246 > https://issues.apache.org/jira/browse/FLUME-2246 > > > Repository: flume-git > > > Description > ------- > > Added configuration options to let user select the max bytes to dump in log > file. Default stays at 16 bytes. > > Change summary > Made LoggerSink implement configurable > user can provide maxByteToDump using configuration > User Guide updated > > > Diffs > ----- > > flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java 128fa84 > flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java > 92ff6fe > flume-ng-doc/sphinx/FlumeUserGuide.rst ae66f89 > > Diff: https://reviews.apache.org/r/16179/diff/ > > > Testing > ------- > > Added test case that sets option to a value higher than 30 and use a string > of length greater than 16 in output. > > > Thanks, > > Ashish Paliwal > >
