> 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
>
>
