Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-8959][tests] Port AccumulatorLiveITCase to flip6 #5719

Closed
wants to merge 4 commits into from

Conversation

zentol
Copy link
Contributor

@zentol zentol commented Mar 19, 2018

Based on #5701.

What is the purpose of the change

This PR ports the AccumulatorLiveITCase to flip6. The existing test was renamed to LegacyAccumulatorLiveITCase, and a ported copy was added.

The heartbeat interval is not configurable for legacy clusters and is hard-coded to 5 seconds. Porting this test (in a reliable fashion) without relying on internal/test APIs would've increased the test duration, so I decided to just leave it as it is.

msg = (TestingJobManagerMessages.UpdatedAccumulators) receiveOne(TIMEOUT);
try {
NotifyingMapper.notifyLatch.await();
Thread.sleep(HEARTBEAT_INTERVAL * 4); // wait for heartbeat
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this test-flakiness waiting to happen? We could run this in a loop and wait until the accumulators are available with a Deadline. If getAccumulators() returned a Future we could even use the utilities in FutureUtils, but can't fix that now, unfortunately... 😓

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well....yeah we could use a deadline and polling, probably will be even faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aljoscha Done; also found a way to make use of FutureUtils.

@aljoscha
Copy link
Contributor

Looks good now, when travis is green! 👌

@tillrohrmann
Copy link
Contributor

Good work @zentol. I guess only a rebase is missing. +1 for merging then.

@zentol
Copy link
Contributor Author

zentol commented Mar 23, 2018

merging.

zentol added a commit to zentol/flink that referenced this pull request Mar 23, 2018
zentol added a commit to zentol/flink that referenced this pull request Mar 23, 2018
zentol added a commit to zentol/flink that referenced this pull request Mar 23, 2018
zentol added a commit to zentol/flink that referenced this pull request Mar 23, 2018
zentol added a commit to zentol/flink that referenced this pull request Mar 23, 2018
zentol added a commit to zentol/flink that referenced this pull request Mar 23, 2018
asfgit pushed a commit that referenced this pull request Mar 23, 2018
@asfgit asfgit closed this in 4f5488c Mar 23, 2018
@zentol zentol deleted the 8959 branch March 23, 2018 18:14
sampathBhat pushed a commit to sampathBhat/flink that referenced this pull request Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants