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