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

Avoid flakiness in test when dropping potentially inexistent table #22109

Merged

Conversation

findinpath
Copy link
Contributor

No description provided.

@cla-bot cla-bot bot added the cla-signed label May 24, 2024
@findinpath findinpath self-assigned this May 24, 2024
@findinpath findinpath requested review from findepi and ebyhr May 24, 2024 10:45
@wendigo wendigo merged commit b061560 into trinodb:master May 24, 2024
19 checks passed
@github-actions github-actions bot added this to the 449 milestone May 24, 2024
@@ -831,7 +831,7 @@ private void runTestCases(String tableName, List<TestCase> testCases)
.collect(joining("), (", "VALUES (", ")")));
}
finally {
getTrinoExecutor().execute("DROP TABLE " + tableName);
getTrinoExecutor().execute("DROP TABLE IF EXISTS " + tableName);
Copy link
Member

@ebyhr ebyhr May 24, 2024

Choose a reason for hiding this comment

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

The table should always exist when reaching this line. Adding IF EXISTS option doesn't make sense for me. Please add the rationale to PR description and commit body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Executor from getTrinoExecutor() (although it has nothing to do with concurrency) mislead me about concurrency.
You are right @ebyhr - the change is quite likely irrelevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

but it's not harmful either

Copy link
Member

Choose a reason for hiding this comment

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

this is a wrong change. If CREATE TABLE succeeded, then we know that the table exists.
Why did we merge this?

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.

4 participants