Re: Review Request 59743: FLUME-3101. When TaildirSource read the file written by high frequency, would not break out to read another file.

[Peter Turcsanyi <tu...@gmail.com>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59743/#review210523
-----------------------------------------------------------



Thank you for the contribution!
This is quite a useful feature, because a file which is being written by high frequency can cause a problem at shutdown too: when Taildir source is continouosly reading a file, it can not answer the shutdown request, so clean shutdown is not possible in this case.

Please find my review comments below that mostly affect the usability (config parameter name / description, logging).


flume-ng-doc/sphinx/FlumeUserGuide.rst
Lines 1189 (patched)
<https://reviews.apache.org/r/59743/#comment295232>

    As Taildir Source reads the files line-by-line, "number of reads" would rather mean the _number of lines_ for me, but actually it controls the _number or batches_ being read consecutively. To make it clear, I would modify the description something like this: "...the number of consecutive batch reads...".
    
    The name of the parameter does not describe what threshold it is about. "maxBatchReads" or similar would be better.



flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirSource.java
Lines 261 (patched)
<https://reviews.apache.org/r/59743/#comment295233>

    According to the suggested `maxBatchReads` parameter name: `bacthReadCount`



flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirSource.java
Line 276 (original), 284 (patched)
<https://reviews.apache.org/r/59743/#comment295230>

    I would handle these two cases separately. In that case the error message would be more straightforward.
    As both cases are normal, info log level is fair enough.


- Peter Turcsanyi


On June 8, 2017, 4:16 p.m., hun shenshi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59743/
> -----------------------------------------------------------
> 
> (Updated June 8, 2017, 4:16 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-doc/sphinx/FlumeUserGuide.rst a5d64f0 
>   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/5/
> 
> 
> Testing
> -------
> 
> mvn clean install -DskipTests -> built
> junit tests for flume-taildir-source module -> passed
> 
> 
> Thanks,
> 
> hun shenshi
> 
>