----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59743/#review177175 -----------------------------------------------------------
Thank you for the patch. In general I think if this problem occurs then the load is higher what the actual Flume setup can handle, so the files won't be fully read unless the load decreases. On the other hand it might be good for some use cases to process other files instead of working on only one, so all in all I agree with the change. I had some comments, could you please fix those issues? Plus it'd be great if you could add this new configuration parameter to the User Guide as well. (`flume-ng-doc/sphinx/FlumeUserGuide.rst`, to check your changes you can generate the html files with the following commands: ``` $ export LC_ALL=C.UTF-8 # Required to build the javadocs on some platforms and in some locales $ mvn clean install -Psite -DskipTests ``` The output will be placed in the `target/site` directory. Let me know if you have any concerns regarding to my comments. flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirSource.java Lines 188 (patched) <https://reviews.apache.org/r/59743/#comment250719> Could you please and a check to handle the invalid (i.e. <= 0) value for the `numThreshold`? Graceful fallback to the default value with some logging could be a good solution, as in the FileChannel: https://github.com/apache/flume/blob/trunk/flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java#L165 flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirSource.java Lines 256 (patched) <https://reviews.apache.org/r/59743/#comment250715> I think it'd be better to use `long` to avoid overflow. flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirSourceConfigurationConstants.java Lines 69 (patched) <https://reviews.apache.org/r/59743/#comment250717> The default value should provide a behaviour which is the same as without this change. Could you please set this to `Integer.MAX_VALUE` (or `Long.MAX_VALUE` according to my comment on line 256)? Or `0` and handle that as special value in the `if` in line 279? flume-ng-sources/flume-taildir-source/src/test/java/org/apache/flume/source/taildir/TestTaildirSource.java Lines 342 (patched) <https://reviews.apache.org/r/59743/#comment250713> Could you please add spaces around the = and < signs? Checkstyle failes on this line. flume-ng-sources/flume-taildir-source/src/test/java/org/apache/flume/source/taildir/TestTaildirSource.java Lines 351 (patched) <https://reviews.apache.org/r/59743/#comment250714> same as in line 342 - Denes Arvay On June 4, 2017, 4:23 p.m., hun shenshi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59743/ > ----------------------------------------------------------- > > (Updated June 4, 2017, 4:23 p.m.) > > > Review request for Flume. > > > Bugs: FLUME-3101 > https://issues.apache.org/jira/browse/FLUME-3101 > > > Repository: flume-git > > > Description > ------- > > Add a threshold in TaildirSource. Use the threshold to control the number of > reads when a file is written by high frequency. > > > Diffs > ----- > > > flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirSource.java > a107a01 > > flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirSourceConfigurationConstants.java > f2347f3 > > flume-ng-sources/flume-taildir-source/src/test/java/org/apache/flume/source/taildir/TestTaildirSource.java > 097ee0b > > > Diff: https://reviews.apache.org/r/59743/diff/3/ > > > Testing > ------- > > mvn clean install -DskipTests -> built > junit tests for flume-taildir-source module -> passed > > > Thanks, > > hun shenshi > >
