You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Denes Arvay <de...@cloudera.com> on 2016/07/01 11:18:13 UTC
Review Request 49453: Patch for FLUME-2725
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49453/
-----------------------------------------------------------
Review request for Flume, Bal�zs Don�t Bessenyei and Attila Simon.
Summary (updated)
-----------------
Patch for FLUME-2725
Bugs: FLUME-2725
https://issues.apache.org/jira/browse/FLUME-2725
Repository: flume-git
Description (updated)
-------
Patch for FLUME-2725 - HDFS Sink does not use configured timezone for rounding
Diffs (updated)
-----
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 21b972b
flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java cc7eac0
Diff: https://reviews.apache.org/r/49453/diff/
Testing (updated)
-------
`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
Re: Review Request 49453: Patch for FLUME-2725
Posted by Mike Percy <mp...@apache.org>.
On July 5, 2016, 10:34 a.m., Denes Arvay wrote:
> > Was it ever considered to introduce jodatime to flume? (http://www.joda.org/joda-time/)
Jodatime is already used in parts of Flume, so we may use it. See for example flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49453/#review140746
-----------------------------------------------------------
On July 1, 2016, 11:18 a.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49453/
> -----------------------------------------------------------
>
> (Updated July 1, 2016, 11:18 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 21b972b
> flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java cc7eac0
>
> 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
>
>
Re: Review Request 49453: Patch for FLUME-2725
Posted by Denes Arvay <de...@cloudera.com>.
> On July 5, 2016, 10:34 a.m., Attila Simon wrote:
> > flume-ng-core/src/main/java/org/apache/flume/formatter/output/BucketPath.java, lines 192-198
> > <https://reviews.apache.org/r/49453/diff/1/?file=1434745#file1434745line192>
> >
> > nit: indention should be 2 spaces
there's a multi line assignment so it's a "continuation indent" which should be 4 spaces.
> On July 5, 2016, 10:34 a.m., Attila Simon wrote:
> > flume-ng-core/src/main/java/org/apache/flume/formatter/output/BucketPath.java, line 239
> > <https://reviews.apache.org/r/49453/diff/1/?file=1434745#file1434745line239>
> >
> > You worried in the corresponding jira about using timezone in rounding might affect users already adopted to the now changed behaviour. Was this concern addressed?
Yes it was. It's a bug fix so we don't need to be backwards compatible.
- Denes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49453/#review140746
-----------------------------------------------------------
On July 1, 2016, 11:18 a.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49453/
> -----------------------------------------------------------
>
> (Updated July 1, 2016, 11:18 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 21b972b
> flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java cc7eac0
>
> 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
>
>
Re: Review Request 49453: Patch for FLUME-2725
Posted by Attila Simon <sa...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49453/#review140746
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/formatter/output/BucketPath.java (lines 192 - 198)
<https://reviews.apache.org/r/49453/#comment206109>
nit: indention should be 2 spaces
flume-ng-core/src/main/java/org/apache/flume/formatter/output/BucketPath.java (line 239)
<https://reviews.apache.org/r/49453/#comment206108>
You worried in the corresponding jira about using timezone in rounding might affect users already adopted to the now changed behaviour. Was this concern addressed?
Was it ever considered to introduce jodatime to flume? (http://www.joda.org/joda-time/)
- Attila Simon
On July 1, 2016, 11:18 a.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49453/
> -----------------------------------------------------------
>
> (Updated July 1, 2016, 11:18 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 21b972b
> flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java cc7eac0
>
> 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
>
>
Re: Review Request 49453: Patch for FLUME-2725
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49453/#review142108
-----------------------------------------------------------
Ship it!
Ship It!
- Mike Percy
On July 13, 2016, 5:29 a.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49453/
> -----------------------------------------------------------
>
> (Updated July 13, 2016, 5:29 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
>
>
Re: Review Request 49453: Patch for FLUME-2725
Posted by Denes Arvay <de...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49453/
-----------------------------------------------------------
(Updated July 13, 2016, 12:29 p.m.)
Review request for Flume, Bal�zs Don�t Bessenyei and Attila Simon.
Changes
-------
Fixes based on Mike's review
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 (updated)
-----
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
Re: Review Request 49453: Patch for FLUME-2725
Posted by Attila Simon <sa...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49453/#review141876
-----------------------------------------------------------
Ship it!
Ship It!
- Attila Simon
On July 12, 2016, 8: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, 8: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
>
>
Re: Review Request 49453: Patch for FLUME-2725
Posted by Denes Arvay <de...@cloudera.com>.
> On July 12, 2016, 11:38 p.m., Mike Percy wrote:
> > flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java, line 35
> > <https://reviews.apache.org/r/49453/diff/2/?file=1442147#file1442147line35>
> >
> > Isn't this just CET? This is the Hungarian time zone, right?
No, it's a nonexistent TZ, GMT + 1 millisecond. I was hesitating using a valid timezone and checking if it's not the system default (and choosing an other one in that case) but this solution seemed cleaner.
> On July 12, 2016, 11:38 p.m., Mike Percy wrote:
> > flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java, line 111
> > <https://reviews.apache.org/r/49453/diff/2/?file=1442147#file1442147line111>
> >
> > Doesn't this assertion fail if you run it while your clock is set to CET?
No, as the `CUSTOM_TIMEZONE` is not CET.
- Denes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49453/#review141976
-----------------------------------------------------------
On July 13, 2016, 12:29 p.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49453/
> -----------------------------------------------------------
>
> (Updated July 13, 2016, 12:29 p.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
>
>
Re: Review Request 49453: Patch for FLUME-2725
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 49453: Patch for FLUME-2725
Posted by Denes Arvay <de...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49453/
-----------------------------------------------------------
(Updated July 12, 2016, 8:36 a.m.)
Review request for Flume, Bal�zs Don�t Bessenyei and Attila Simon.
Changes
-------
rebased on trunk + fixed checkstyle errors
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 (updated)
-----
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
Re: Review Request 49453: Patch for FLUME-2725
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49453/#review141772
-----------------------------------------------------------
Mind rebasing on trunk? If you run mvn clean verify -DskipTests -Drat.skip then checkstyle will run and you will know if there are any style violations. At least, that is the fastest way I know of to do the style check.
I'll wait for Attila to finish reviewing this before picking it back up myself for a final review.
- Mike Percy
On July 1, 2016, 4:18 a.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49453/
> -----------------------------------------------------------
>
> (Updated July 1, 2016, 4:18 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 21b972b
> flume-ng-core/src/test/java/org/apache/flume/tools/TestTimestampRoundDownUtil.java cc7eac0
>
> 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
>
>