You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Balázs Donát Bessenyei <be...@cloudera.com> on 2016/10/04 11:11:30 UTC
Review Request 52510: Some tests in TestBucketWriter are flaky
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52510/
-----------------------------------------------------------
Review request for Flume.
Bugs: FLUME-3002
https://issues.apache.org/jira/browse/FLUME-3002
Repository: flume-git
Description
-------
testFileSuffixNotGiven (and probably a few other tests) are flaky
Diffs
-----
c/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java b096410
c/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java 742deb0
Diff: https://reviews.apache.org/r/52510/diff/
Testing
-------
mvn clean install runs successfully in flume-ng-sinks
Thanks,
Bal�zs Don�t Bessenyei
Re: Review Request 52510: Some tests in TestBucketWriter are flaky
Posted by Attila Simon <sa...@cloudera.com>.
> On Oct. 21, 2016, 1:21 p.m., Attila Simon wrote:
> > Ship It!
Discussed offline that there will be a follow up patch to use builder pattern for constructing BucketWriter. (So that further testing only instance mutators can be eliminated)
- Attila
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52510/#review153538
-----------------------------------------------------------
On Oct. 4, 2016, 11:11 a.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52510/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2016, 11:11 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-3002
> https://issues.apache.org/jira/browse/FLUME-3002
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> testFileSuffixNotGiven (and probably a few other tests) are flaky
>
>
> Diffs
> -----
>
> c/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java b096410
> c/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java 742deb0
>
> Diff: https://reviews.apache.org/r/52510/diff/
>
>
> Testing
> -------
>
> mvn clean install runs successfully in flume-ng-sinks
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>
Re: Review Request 52510: Some tests in TestBucketWriter are flaky
Posted by Attila Simon <sa...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52510/#review153538
-----------------------------------------------------------
Ship it!
Ship It!
- Attila Simon
On Oct. 4, 2016, 11:11 a.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52510/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2016, 11:11 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-3002
> https://issues.apache.org/jira/browse/FLUME-3002
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> testFileSuffixNotGiven (and probably a few other tests) are flaky
>
>
> Diffs
> -----
>
> c/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java b096410
> c/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java 742deb0
>
> Diff: https://reviews.apache.org/r/52510/diff/
>
>
> Testing
> -------
>
> mvn clean install runs successfully in flume-ng-sinks
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>
Re: Review Request 52510: Some tests in TestBucketWriter are flaky
Posted by Denes Arvay <de...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52510/#review153678
-----------------------------------------------------------
Ship it!
Ship It!
- Denes Arvay
On Oct. 4, 2016, 1:11 p.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52510/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2016, 1:11 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-3002
> https://issues.apache.org/jira/browse/FLUME-3002
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> testFileSuffixNotGiven (and probably a few other tests) are flaky
>
>
> Diffs
> -----
>
> c/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java b096410
> c/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java 742deb0
>
> Diff: https://reviews.apache.org/r/52510/diff/
>
>
> Testing
> -------
>
> mvn clean install runs successfully in flume-ng-sinks
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>
Re: Review Request 52510: Some tests in TestBucketWriter are flaky
Posted by Balázs Donát Bessenyei <be...@cloudera.com>.
> On Oct. 18, 2016, 4:26 p.m., Attila Simon wrote:
> > Ahh now I see what is going on. BucketWriter instantiate a Clock class which is in turn used in the file name counter. The test first instantiates the BucketWriter - which initialize the filename counter -then overrides the Clock but since Clock is not used in BucketWriter directly - only via the file name counter at instantiation time so it won't be updated with the override - the test failes as it checks the file name.
> > So actually a single extra line into the `BucketWriter.setClock(Clock clock)` method would solve the issue:
> > ```
> > void setClock(Clock clock) {
> > this.clock = clock;
> > this.fileExtensionCounter.set(clock.currentTimeMillis());
> > }
> > ```
> > This way the test would update the relevant information - file name counter - which then in turn can be checked.
> >
> > Could you please consider simplifying your change?
Thank you for the review!
Even though the change you have suggested does fix the flakiness of the tests (which is awesome!), I am not sure people would expect the caused side-effect in void setClock(Clock clock) -> fileExtensionCounter.
The change in its current form does eliminate private Clock clock which is "nice". However, it also eliminates void setClock(Clock clock) which helps avoiding violation of the monotony of fileExtensionCounter (which is assumed), thus it improves the integrity of BucketWriter (encapsulation).
Please, let me know your thoughts.
- Bal�zs Don�t
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52510/#review153104
-----------------------------------------------------------
On Oct. 4, 2016, 11:11 a.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52510/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2016, 11:11 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-3002
> https://issues.apache.org/jira/browse/FLUME-3002
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> testFileSuffixNotGiven (and probably a few other tests) are flaky
>
>
> Diffs
> -----
>
> c/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java b096410
> c/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java 742deb0
>
> Diff: https://reviews.apache.org/r/52510/diff/
>
>
> Testing
> -------
>
> mvn clean install runs successfully in flume-ng-sinks
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>
Re: Review Request 52510: Some tests in TestBucketWriter are flaky
Posted by Attila Simon <sa...@cloudera.com>.
> On Oct. 18, 2016, 4:26 p.m., Attila Simon wrote:
> > Ahh now I see what is going on. BucketWriter instantiate a Clock class which is in turn used in the file name counter. The test first instantiates the BucketWriter - which initialize the filename counter -then overrides the Clock but since Clock is not used in BucketWriter directly - only via the file name counter at instantiation time so it won't be updated with the override - the test failes as it checks the file name.
> > So actually a single extra line into the `BucketWriter.setClock(Clock clock)` method would solve the issue:
> > ```
> > void setClock(Clock clock) {
> > this.clock = clock;
> > this.fileExtensionCounter.set(clock.currentTimeMillis());
> > }
> > ```
> > This way the test would update the relevant information - file name counter - which then in turn can be checked.
> >
> > Could you please consider simplifying your change?
>
> Bal�zs Don�t Bessenyei wrote:
> Thank you for the review!
>
> Even though the change you have suggested does fix the flakiness of the tests (which is awesome!), I am not sure people would expect the caused side-effect in void setClock(Clock clock) -> fileExtensionCounter.
>
> The change in its current form does eliminate private Clock clock which is "nice". However, it also eliminates void setClock(Clock clock) which helps avoiding violation of the monotony of fileExtensionCounter (which is assumed), thus it improves the integrity of BucketWriter (encapsulation).
>
> Please, let me know your thoughts.
Clock is only used to initialize file extension counter (so there is initialization phase binding between these two). I just made sure this expected binding is maintained in the setter as well through the lifetime of a BucketWriter. I may suggest setClock should be also tagged with @VisibleForTesting (it is already scoped with package auto) to make it clear that it is only for tests (which is the case now by checking its usages).
- Attila
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52510/#review153104
-----------------------------------------------------------
On Oct. 4, 2016, 11:11 a.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52510/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2016, 11:11 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-3002
> https://issues.apache.org/jira/browse/FLUME-3002
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> testFileSuffixNotGiven (and probably a few other tests) are flaky
>
>
> Diffs
> -----
>
> c/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java b096410
> c/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java 742deb0
>
> Diff: https://reviews.apache.org/r/52510/diff/
>
>
> Testing
> -------
>
> mvn clean install runs successfully in flume-ng-sinks
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>
Re: Review Request 52510: Some tests in TestBucketWriter are flaky
Posted by Attila Simon <sa...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52510/#review153104
-----------------------------------------------------------
Ahh now I see what is going on. BucketWriter instantiate a Clock class which is in turn used in the file name counter. The test first instantiates the BucketWriter - which initialize the filename counter -then overrides the Clock but since Clock is not used in BucketWriter directly - only via the file name counter at instantiation time so it won't be updated with the override - the test failes as it checks the file name.
So actually a single extra line into the `BucketWriter.setClock(Clock clock)` method would solve the issue:
```
void setClock(Clock clock) {
this.clock = clock;
this.fileExtensionCounter.set(clock.currentTimeMillis());
}
```
This way the test would update the relevant information - file name counter - which then in turn can be checked.
Could you please consider simplifying your change?
- Attila Simon
On Oct. 4, 2016, 11:11 a.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52510/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2016, 11:11 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-3002
> https://issues.apache.org/jira/browse/FLUME-3002
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> testFileSuffixNotGiven (and probably a few other tests) are flaky
>
>
> Diffs
> -----
>
> c/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java b096410
> c/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java 742deb0
>
> Diff: https://reviews.apache.org/r/52510/diff/
>
>
> Testing
> -------
>
> mvn clean install runs successfully in flume-ng-sinks
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>
Re: Review Request 52510: Some tests in TestBucketWriter are flaky
Posted by Attila Simon <sa...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52510/#review153076
-----------------------------------------------------------
Could you please give us some details about the idea of this change? Ie some high level description about what was the problem and how you solved it.
- Attila Simon
On Oct. 4, 2016, 11:11 a.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52510/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2016, 11:11 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-3002
> https://issues.apache.org/jira/browse/FLUME-3002
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> testFileSuffixNotGiven (and probably a few other tests) are flaky
>
>
> Diffs
> -----
>
> c/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java b096410
> c/flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java 742deb0
>
> Diff: https://reviews.apache.org/r/52510/diff/
>
>
> Testing
> -------
>
> mvn clean install runs successfully in flume-ng-sinks
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>