----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50378/#review144092 -----------------------------------------------------------
flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (line 150) <https://reviews.apache.org/r/50378/#comment210092> terminology "weakly consistent" is not defined by javadocs of SimpleFileVisitor flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (line 209) <https://reviews.apache.org/r/50378/#comment210093> terminology "weakly consistent" is not defined by javadocs of SimpleFileVisitor flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (line 212) <https://reviews.apache.org/r/50378/#comment210094> terminology "weakly consistent" is not defined by javadocs of SimpleFileVisitor flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (line 220) <https://reviews.apache.org/r/50378/#comment210095> should be linked https://docs.oracle.com/javase/8/docs/api/java/nio/file/FileVisitor.html instead. flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (line 225) <https://reviews.apache.org/r/50378/#comment210097> Is this solution follow symlinks? If not then it would be a breaking change. flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (lines 226 - 228) <https://reviews.apache.org/r/50378/#comment210110> performance downgrade due to the idempotent instantiations of matchers flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (lines 227 - 237) <https://reviews.apache.org/r/50378/#comment210108> Missing short circuit on surely not matching huge subdirs. You need to override the preVisitDirectory for that. (Also I would recommend overriding visitFileFailed for robustness) flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (lines 254 - 274) <https://reviews.apache.org/r/50378/#comment210087> Usage of magic constants like '/', '\', "*?[{" most be avoided. flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (lines 254 - 274) <https://reviews.apache.org/r/50378/#comment210090> wouldn't this be easier like any of this below: String trimPathBeforeFirstWildcard2(String path) { final String WILDCARDS = "*?{["; String[] dirs = path.split(File.pathSeparator); List<String> result = new LinkedList<String>(); for (int i = 0; i < dirs.length; ++i) { if (StringUtils.containsAny(dirs[i], WILDCARDS)) { result.add(dirs[i]); } } if (result.isEmpty()) { return File.separator; } else { return StringUtils.join(result, File.separator); } } String trimPathBeforeFirstWildcard3(String path) { final String WILDCARDS = "*?{["; int endDirIdx = 0; for(int i = 0; i < path.length(); ++i) { char at = path.charAt(i); if (at == File.separatorChar) { endDirIdx = i; } else if (StringUtils.contains(WILDCARDS, at)) { break; } else { continue; } } if(endDirIdx > 0){ return path.substring(0, endDirIdx); }else { return File.separator; } } flume-ng-sources/flume-taildir-source/src/main/java/org/apache/flume/source/taildir/TaildirMatcher.java (line 260) <https://reviews.apache.org/r/50378/#comment210088> could you please explain why the second half was needed? (i >= 1 && path.charAt(i - 1) != '\') flume-ng-sources/flume-taildir-source/src/test/java/org/apache/flume/source/taildir/TestTaildirSource.java (lines 148 - 157) <https://reviews.apache.org/r/50378/#comment210102> would you mind linking the syntax explanation instead of copying it here? flume-ng-sources/flume-taildir-source/src/test/java/org/apache/flume/source/taildir/TestTaildirSource.java (line 161) <https://reviews.apache.org/r/50378/#comment210103> I think you just testing the filtering with this junit test. This can be done as well in the TestTaildirMatcher file without the need of creating a channel and transaction. Required complexity of such test would be closer to the unit idea. I think it is getting there to be committed. Some more iteration and it will be done. Keep up the good work! - Attila Simon On July 29, 2016, 12:04 p.m., qiao wen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50378/ > ----------------------------------------------------------- > > (Updated July 29, 2016, 12:04 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 3f08d8b > > 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/TestTaildirMatcher.java > c341054 > > 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 > >
