----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50378/#review143526 -----------------------------------------------------------
flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (line 123) <https://reviews.apache.org/r/50378/#comment209341> could you please add to the documentation that if there is a wildcard in the directory name then the `cachePatternMatching` is forced to false? flume-ng-doc/sphinx/FlumeUserGuide.rst (line 1123) <https://reviews.apache.org/r/50378/#comment209343> A bit more explicit description would be better, e.g. "Glob pattern wildcards (see: -> link to https://docs.oracle.com/javase/7/docs/api/java/nio/file/FileSystem.html#getPathMatcher(java.lang.String) ) are allowed..." flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (line 223) <https://reviews.apache.org/r/50378/#comment209362> it's not needed to implement this method, the super implementation is basically the same (does some not-null checks and returns `CONTINUE`) flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (lines 238 - 241) <https://reviews.apache.org/r/50378/#comment209385> it would be good to log the exception instead of silently swallowing it. Something similar as in line 244 flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (lines 250 - 270) <https://reviews.apache.org/r/50378/#comment209365> Please write some test for this function. It seems to work as expected but I'd be more confident if there were some tests for the edge cases at least (e.g. expression character at the beginning, etc). And it would also make sure that things won't brake if this method changes. flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (line 251) <https://reviews.apache.org/r/50378/#comment209368> The name of this method doesn't really describe what it does. (finding the first wildcard and trimming the path) something like `trimPathBeforeWildcards` would be better flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (line 255) <https://reviews.apache.org/r/50378/#comment209347> In general I think this approach is not the best, as it has to iterate over the `"*?[{"` string every time, whereas with a `Set` it could be faster. As the length of the pattern string is only 4, it mightn't be worth the change, mostly because of the char->Character autoboxing's overhead. But with a `BitSet` we could have a bit (no pun intended) better performace (although I don't have numbers): ``` private static final BitSet GLOB_WILDCARD_BIT_SET = new BitSet(8); static { for (char ch : "*?[{".toCharArray()) { GLOB_WILDCARD_BIT_SET.set(ch); } } ``` and then check with `GLOB_WILDCARD_BIT_SET.get(path.charAt(i))` But again, I just wanted to share my opinion with you, I'm definitely not insisting on this change. flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (line 263) <https://reviews.apache.org/r/50378/#comment209348> `== true` is not needed, the code is cleaner without it. flume-ng-sources/flume-taildir-source/src/test/java/org/apache/flume/source/taildir/TestTaildirSource.java (line 147) <https://reviews.apache.org/r/50378/#comment209374> could you please add some javadoc to explain the purpose of this test? flume-ng-sources/flume-taildir-source/src/test/java/org/apache/flume/source/taildir/TestTaildirSource.java (line 148) <https://reviews.apache.org/r/50378/#comment209350> could you please simplify this by extracting the repeated lines to a method? eg: `private File createTempFile(String path)` which could create the parent dir, the file itself and write the content. The content can be the absolute path for example, in that case we don't need an extra parameter for that, and the test could do `assertTrue(out.contains(f.getAbsolutePath()));` flume-ng-sources/flume-taildir-source/src/test/java/org/apache/flume/source/taildir/TestTaildirSource.java (line 151) <https://reviews.apache.org/r/50378/#comment209351> it's better to use the `File(File parent, String child)` constructor, e.g. `File f1 = new File(tmpDir, "fg1/dir1/subdir/file1.txt");` instead of string concatenation. flume-ng-sources/flume-taildir-source/src/test/java/org/apache/flume/source/taildir/TestTaildirSource.java (line 241) <https://reviews.apache.org/r/50378/#comment209376> could you please add some javadoc here too? flume-ng-sources/flume-taildir-source/src/test/java/org/apache/flume/source/taildir/TestTaildirSource.java (lines 268 - 270) <https://reviews.apache.org/r/50378/#comment209355> are these `sleep`s really needed? Thanks for the patch. Besides the line-comments I've added there are a handful of minor formatting issues which make the checkstyle fail, please fix those too. You can run the checkstyle with `mvn clean verify -DskipTests -Drat.skip`. Let me know if you have any issues running it, I'm happy to help. - Denes Arvay On July 25, 2016, 3:40 p.m., qiao wen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50378/ > ----------------------------------------------------------- > > (Updated July 25, 2016, 3:40 p.m.) > > > Review request for Flume. > > > Repository: flume-git > > > Description > ------- > > In our log management project, we wan't to track many log files like this: > /app/dir1/log.* > /app/dir2/log.* > ... > /app/dirn/log.* > But TaildirSource can't support wildcards in filegroup directory name. The > following config is expected: > a1.sources.r1.filegroups.fg = /app/*/log.* > > > Diffs > ----- > > flume-ng-doc/sphinx/FlumeUserGuide.rst 1334500 > > flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java > ad9f720 > > flume-ng-sources/flume-taildir-source/src/test/java/org/apache/flume/source/taildir/TestTaildirSource.java > 097ee0b > > Diff: https://reviews.apache.org/r/50378/diff/ > > > Testing > ------- > > All tests in TestTaildirSource passed. > > > Thanks, > > qiao wen > >
