----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49025/#review139356 -----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java (line 215) <https://reviews.apache.org/r/49025/#comment204525> style nit: don't leave spaces inside the parens. This should look like: if (candidate.isDirectory()) { See http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449 flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java (line 217) <https://reviews.apache.org/r/49025/#comment204526> style nit: same here. also get rid of the unnecessary parenthesis. Write this as: if (!recursiveDirectorySearch || directoryName.startsWith(".") || ignorePattern.matcher(directoryName).matches()) { Yeah, I know the code was moved and it already looked like that. flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java (line 224) <https://reviews.apache.org/r/49025/#comment204568> There is no need for an else here because the function short-circuits and returns in all cases in the "if" clause. Just remove the else clause and you can also reduce the indentation of the rest of the code by a couple of spaces. flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java (line 226) <https://reviews.apache.org/r/49025/#comment204528> nit: more spurious parentheses. Also the indentation could use some cleanup, i.e. align the clauses of the "if" flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java (line 241) <https://reviews.apache.org/r/49025/#comment204529> nit: missing space between ) and { Also, there are 2 spaces between "private" and "List" flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java (line 243) <https://reviews.apache.org/r/49025/#comment204531> Can we just prevent null values here? How about just add a Preconditions.checkNotNull(directory) at the top of the method? also, nit: add spaces around == but not in front of the !, like: if (directory == null || !directory.isDirectory()) { There is also a missing space before the curly brace. flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java (line 247) <https://reviews.apache.org/r/49025/#comment204533> nit: spacing issues on this line as mentioned elsewhere in this review flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java (line 251) <https://reviews.apache.org/r/49025/#comment204534> nit: else on same line flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java (line 487) <https://reviews.apache.org/r/49025/#comment204567> nit: I know you didn't write this but please indent these clauses by 4 spaces for readability flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySourceConfigurationConstants.java (line 97) <https://reviews.apache.org/r/49025/#comment204537> Nit: can you make the comment a little prettier? Like: /** Flag to indicate if we should recursively checking for new files. The * default is false, so a configuration file entry would be needed to enable * this setting */ flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 44) <https://reviews.apache.org/r/49025/#comment204536> Unused import? flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 81) <https://reviews.apache.org/r/49025/#comment204535> nit: need space before { flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 83) <https://reviews.apache.org/r/49025/#comment204538> nit: space before { flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 87) <https://reviews.apache.org/r/49025/#comment204545> nit: The "else" should be on the same line as the curly braces in Java. Like: } else { See http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449 flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 94) <https://reviews.apache.org/r/49025/#comment204548> Hmmm. Seems fishy. Why are we doing this? flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 98) <https://reviews.apache.org/r/49025/#comment204546> nit: needs a space before the { flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 206) <https://reviews.apache.org/r/49025/#comment204539> Please add a comment describing this unit test flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 209) <https://reviews.apache.org/r/49025/#comment204562> nit: Use the 2-arg File constructor instead of a string concatenation. Like: File subDir = new File(tmpDir, "directorya/directoryb/directoryc"); flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 210) <https://reviews.apache.org/r/49025/#comment204563> nit: How about just write this as: assertTrue(subDir.mkdirs()); flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 211) <https://reviews.apache.org/r/49025/#comment204564> See above, this line seems unnecessarily verbose. If we get an assertion it will tell us the line of the failure flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 213) <https://reviews.apache.org/r/49025/#comment204561> style: need spaces around the assignment operator flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 214) <https://reviews.apache.org/r/49025/#comment204565> nit: Use 2-arg File constructor flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 231) <https://reviews.apache.org/r/49025/#comment204549> Can we remove this sleep? It slows down the tests and can cause them to be flaky. How about just a loop trying to get the event we want, and have it retry up to some max timeout, say 30 seconds. flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 235) <https://reviews.apache.org/r/49025/#comment204555> Can we check that the body contents are what we expected as well? flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 237) <https://reviews.apache.org/r/49025/#comment204553> nit: Super long line. Please wrap it at 100 chars or so. flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 241) <https://reviews.apache.org/r/49025/#comment204540> Nit: Extra space, just leave one line flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 243) <https://reviews.apache.org/r/49025/#comment204544> Please add a comment describing the unit test flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 248) <https://reviews.apache.org/r/49025/#comment204557> Please don't use println. Use a LOGGER flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 250) <https://reviews.apache.org/r/49025/#comment204558> Same as above. But seems like printing is pointless. It should be an assertion. flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 252) <https://reviews.apache.org/r/49025/#comment204559> nit: extra blank line flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 256) <https://reviews.apache.org/r/49025/#comment204543> nit: We typically use 4 space indent on line continuations, unless we align the indentation with the argument above it. e.g. either: Files.write("file1line1\nfile1line2\nfile1line3\nfile1line4\n" + "file1line5\nfile1line6\nfile1line7\nfile1line8\n", f1, Charsets.UTF_8); or Files.write("file1line1\nfile1line2\nfile1line3\nfile1line4\n" + "file1line5\nfile1line6\nfile1line7\nfile1line8\n", f1, Charsets.UTF_8); Please pick one here and elsewhere flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 259) <https://reviews.apache.org/r/49025/#comment204560> nit: please remove extra blank line (one should suffice) flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 271) <https://reviews.apache.org/r/49025/#comment204551> nit: please add a space after the // which is how the rest of the codebase does comments flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 272) <https://reviews.apache.org/r/49025/#comment204554> Nit: super long line. Pls wrap flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 273) <https://reviews.apache.org/r/49025/#comment204552> Remove the sleep here too, just retry for up to 200ms or so. If this test gets flaky hopefully we will notice. flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 275) <https://reviews.apache.org/r/49025/#comment204550> nit: Please make the spacing and indentation of the comment here consistent, it's pretty weird right now. Also, the info in this comment here is a good candidate for a top-of-the-test comment instead of being buried in the middle. flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 287) <https://reviews.apache.org/r/49025/#comment204541> nit: extra space, same as above flume-ng-doc/sphinx/FlumeUserGuide.rst (line 1003) <https://reviews.apache.org/r/49025/#comment204524> Seems like the indentation is off here. Also please get rid of the tab character. Overall looks pretty good, except I don't get why we have to use reflection in the test and I found a bunch of nits. - Mike Percy On June 22, 2016, 12:24 p.m., Balázs Donát Bessenyei wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49025/ > ----------------------------------------------------------- > > (Updated June 22, 2016, 12:24 p.m.) > > > Review request for Flume, Denes Arvay, Mike Percy, and Attila Simon. > > > Repository: flume-git > > > Description > ------- > > SpoolrDir currently monitors a directory and can not handle sub-directories. > This JIRA is to make SpoolDir able to walk down a source directory and > monitor new files. > > > Diffs > ----- > > > flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java > d54f415 > > flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java > 3fe947d > > flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySourceConfigurationConstants.java > 5053697 > > flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java > fe530ff > flume-ng-doc/sphinx/FlumeUserGuide.rst 74d2887 > > Diff: https://reviews.apache.org/r/49025/diff/ > > > Testing > ------- > > Ran tests before the patch: > # mvn clean install -DskipTests -Drat.skip=true; mvn -pl flume-ng-core > -Drat.skip=true test > Tests run: 378, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 07:55 min > [INFO] Finished at: 2016-06-21T16:13:46+02:00 > [INFO] Final Memory: 35M/510M > [INFO] > ------------------------------------------------------------------------ > > After patch: > # mvn clean install -DskipTests -Drat.skip=true; mvn -pl flume-ng-core > -Drat.skip=true test > Tests run: 380, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 06:18 min > [INFO] Finished at: 2016-06-21T17:04:17+02:00 > [INFO] Final Memory: 35M/511M > [INFO] > ------------------------------------------------------------------------ > > Patch also includes docs > > > Thanks, > > Balázs Donát Bessenyei > >
