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

Fix flaky TestExchangeClient.testAddLocation #10664

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

arhimondr
Copy link
Contributor

Wait for the task3 to finish after closing the client

@cla-bot cla-bot bot added the cla-signed label Jan 18, 2022
@arhimondr arhimondr requested review from losipiuk and findepi January 18, 2022 17:04
@arhimondr
Copy link
Contributor Author

Resolves #10583

@@ -294,14 +294,14 @@ public void testAddLocation()
assertTaskIsNotFinished(buffer, task3);

exchangeClient.close();
buffer.whenTaskFinished(task3).get(10, SECONDS);
assertThat(buffer.getFinishedTasks()).containsExactlyInAnyOrder(task1, task2, task3);
assertThat(buffer.getFailedTasks().asMap()).isEmpty();
Copy link
Member

Choose a reason for hiding this comment

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

do we still need assertEventually below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the current implementation it looks like it isn't necessary

Currently the future is assigned before clientCallback.clientFinished

               synchronized (HttpPageBufferClient.this) {
                    closed = true;
                    if (future == resultFuture) {
                        future = null;
                    }
                    lastUpdate = DateTime.now();
                }
                requestsCompleted.incrementAndGet();
                clientCallback.clientFinished(HttpPageBufferClient.this);

However I'm not sure whether it makes sense to implicitly rely on this fact, as it may change in the future and the test may silently become flaky again.

Wait for the task3 to finish after closing the client
@arhimondr arhimondr force-pushed the fix-add-locations-test branch from cfaa4bd to 77bfa97 Compare January 19, 2022 17:00
@losipiuk losipiuk merged commit f009a46 into trinodb:master Jan 20, 2022
@github-actions github-actions bot added this to the 369 milestone Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants