----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49453/#review141976 -----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/tools/TimestampRoundDownUtil.java (line 26) <https://reviews.apache.org/r/49453/#comment207436> Please add annotations to this class: @Private and @Evolving flume-ng-core/src/main/java/org/apache/flume/tools/TimestampRoundDownUtil.java (line 143) <https://reviews.apache.org/r/49453/#comment207449> I think adding parenthesis around the condition in the ternary operator here would make this line more readable. Would you mind doing that? e.g.: Calendar cal = (timeZone == null) ? Calendar.getInstance() : Calendar.getInstance(timeZone); flume-ng-core/src/test/java/org/apache/flume/formatter/output/TestBucketPath.java (line 57) <https://reviews.apache.org/r/49453/#comment207450> nit: You can use Java 7 syntax for the generic and make it new HashMap<>(); flume-ng-core/src/test/java/org/apache/flume/formatter/output/TestBucketPath.java (line 93) <https://reviews.apache.org/r/49453/#comment207438> Please add a short comment describing the purpose of this test. flume-ng-core/src/test/java/org/apache/flume/formatter/output/TestBucketPath.java (line 127) <https://reviews.apache.org/r/49453/#comment207439> Needs test comment flume-ng-core/src/test/java/org/apache/flume/formatter/output/TestBucketPath.java (line 133) <https://reviews.apache.org/r/49453/#comment207445> Can you reduce the copy / paste in this test? flume-ng-core/src/test/java/org/apache/flume/formatter/output/TestBucketPath.java (line 155) <https://reviews.apache.org/r/49453/#comment207440> Test comment flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java (line 35) <https://reviews.apache.org/r/49453/#comment207434> Isn't this just CET? This is the Hungarian time zone, right? flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java (line 54) <https://reviews.apache.org/r/49453/#comment207441> test comment flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java (line 56) <https://reviews.apache.org/r/49453/#comment207444> I see lines 56-64 duplicated in L95-L103 (and actually maybe even more code than that) Can you extract a method or two to reduce the line count of this patch? flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java (line 93) <https://reviews.apache.org/r/49453/#comment207442> test comment flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java (line 111) <https://reviews.apache.org/r/49453/#comment207435> Doesn't this assertion fail if you run it while your clock is set to CET? flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java (line 132) <https://reviews.apache.org/r/49453/#comment207443> test comment - Mike Percy On July 12, 2016, 1:36 a.m., Denes Arvay wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49453/ > ----------------------------------------------------------- > > (Updated July 12, 2016, 1:36 a.m.) > > > Review request for Flume, Balázs Donát Bessenyei and Attila Simon. > > > Bugs: FLUME-2725 > https://issues.apache.org/jira/browse/FLUME-2725 > > > Repository: flume-git > > > Description > ------- > > Patch for FLUME-2725 - HDFS Sink does not use configured timezone for rounding > > > Diffs > ----- > > > flume-ng-core/src/main/java/org/apache/flume/formatter/output/BucketPath.java > b2fe3f0 > > flume-ng-core/src/main/java/org/apache/flume/tools/TimestampRoundDownUtil.java > daa9606 > > flume-ng-core/src/test/java/org/apache/flume/formatter/output/TestBucketPath.java > b1b828a > > flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java > 1ac11ab > > Diff: https://reviews.apache.org/r/49453/diff/ > > > Testing > ------- > > `org.apache.flume.formatter.output.TestBucketPath` and > `org.apache.flume.tools.TestTimestampRoundDownUtil` were extended with new > methods testing with `TimeZone`. Existing and new tests pass. > > > Thanks, > > Denes Arvay > >
