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.

Sign up for GitHub

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

Jump to bottom

Re-enable Nessie namespace/table visibility tests #18124

Merged
merged 1 commit into from
Nov 27, 2022
Merged

Re-enable Nessie namespace/table visibility tests #18124

merged 1 commit into from
Nov 27, 2022

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Jul 29, 2022 •
edited
Loading

testTableDataVisibility() and testNamespaceVisibility are both using the same namespace names (where namespace_one is created on the main branch), so it's possible that those tests interfere with each other if the tests run in parallel

@nastra nastra requested a review from a team as a code owner July 29, 2022 12:38
@nastra nastra requested a review from presto-oss July 29, 2022 12:38
@nastra nastra closed this Jul 29, 2022
@nastra nastra reopened this Jul 29, 2022
@nastra nastra closed this Jul 29, 2022
@nastra nastra reopened this Jul 29, 2022
@dimas-b dimas-b mentioned this pull request Jul 29, 2022
Closed
beinan
beinan reviewed Jul 29, 2022
View reviewed changes
Copy link
Member

@beinan beinan left a comment

Choose a reason for hiding this comment

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

Thank you so much for fixing this! Just one non-blocking comments, is it possible to do some clean up after or before each test? otherwise these tests are still brittle especially when we add new tests with the same namespace name in the future.

@nastra
Copy link
Contributor Author

nastra commented Aug 1, 2022 •
edited
Loading

@beinan yes that makes perfect sense to do that. I added some cleanup after each test that deletes all branches/tags and also improved the overall isolation in the tests so that parallel runs don't interfere with each other

@nastra nastra closed this Aug 1, 2022
@nastra nastra reopened this Aug 1, 2022
@nastra nastra force-pushed the nessie-tests branch 3 times, most recently from b3c4522 to 366d0e7 Compare August 1, 2022 14:10
@nastra
Copy link
Contributor Author

nastra commented Aug 1, 2022

@beinan turns out this isn't as trivial as I thought, because the query runner with nessie is usually created at the class level, while we clean up and therefore modify the underlying references at the test level (which means we're modifying the underlying state of the query runner).
@ajantha-bhat can you please do a follow-up on this PR and rewrite the test in a way so that everything is properly cleaned up and tests don't interfere with each other when they run in parallel?

@ajantha-bhat
Copy link
Contributor

@ajantha-bhat can you please do a follow-up on this PR and rewrite the test in a way so that everything is properly cleaned up and tests don't interfere with each other when they run in parallel?

Sure. I can check in my free time.

@rohanpednekar
Copy link
Contributor

ping @ajantha-bhat!

@ajantha-bhat
Copy link
Contributor

@rohanpednekar : Thanks for the remainder. I will handle it this week.

@nastra nastra force-pushed the nessie-tests branch 3 times, most recently from 1affedc to 9d5ed68 Compare October 20, 2022 09:22
nastra
nastra commented Oct 20, 2022
View reviewed changes
presto-testing-docker/src/main/java/com/facebook/presto/testing/containers/NessieContainer.java
@@ -27,7 +27,7 @@
{
private static final Logger log = Logger.get(NessieContainer.class);

public static final String DEFAULT_IMAGE = "projectnessie/nessie:0.30.0";
public static final String DEFAULT_IMAGE = "projectnessie/nessie:0.43.0";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just aligning the nessie version with the one being used in the presto-iceberg pom

@nastra
Copy link
Contributor Author

nastra commented Oct 20, 2022

@beinan turns out this isn't as trivial as I thought, because the query runner with nessie is usually created at the class level, while we clean up and therefore modify the underlying references at the test level (which means we're modifying the underlying state of the query runner). @ajantha-bhat can you please do a follow-up on this PR and rewrite the test in a way so that everything is properly cleaned up and tests don't interfere with each other when they run in parallel?

It appears that @Test(singleThreaded = true) fixed the issue I was seeing, so this should be ready for a re-review @beinan

ajantha-bhat
ajantha-bhat reviewed Oct 20, 2022
View reviewed changes
presto-iceberg/src/test/java/com/facebook/presto/iceberg/nessie/TestNessieMultiBranching.java
@@ -38,6 +40,7 @@
import static com.facebook.presto.tests.QueryAssertions.assertEqualsIgnoreOrder;
import static org.assertj.core.api.Assertions.assertThat;

@Test(singleThreaded = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you have made it single-threaded execution because resetData will clean all the branches including the concurrently executing test case's branches.

So, maybe the efficient way is to have testcase level clean-up which only drop the branches it created (instead of generic cleanup) and ensuring none of the testcase commits on default branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a fan of doing cleanup within a test method itself, because you'd have to wrap the entire test method code in a try-finally to guarantee that cleanup runs at the end of the method.
This test class has 3 tests in total, so running this single-threaded just makes the test more predictable and less prone to concurrent cleanup/execution and shouldn't affect CI time too much

Copy link
Contributor

Choose a reason for hiding this comment

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

It is just 3 test cases. So, I don't mind single-threaded execution.
The current implementation sounds good to me.

@ajantha-bhat
Copy link
Contributor

@beinan, @rohanpednekar: We have added the clean-up in this PR itself today. So, there won't any follow PR from me for the same.

@nastra nastra requested review from beinan and removed request for presto-oss October 25, 2022 05:42
@nastra
Copy link
Contributor Author

nastra commented Oct 27, 2022

@beinan could you re-review this one please?

beinan
beinan approved these changes Oct 29, 2022
View reviewed changes
Copy link
Member

@beinan beinan left a comment

Choose a reason for hiding this comment

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

lgtm, @ChunxuTang do you want to take a look also?

@nastra
Copy link
Contributor Author

nastra commented Nov 3, 2022

@beinan @ChunxuTang is this something that we can merge?

@nastra
Re-enable Nessie namespace/table visibility tests
f616e32 
This also cleans up branches/tags after each test.
@nastra nastra closed this Nov 14, 2022
@nastra nastra reopened this Nov 14, 2022
@nastra nastra closed this Nov 14, 2022
@nastra nastra reopened this Nov 14, 2022
@nastra nastra closed this Nov 14, 2022
@nastra nastra deleted the nessie-tests branch November 14, 2022 11:26
@nastra nastra restored the nessie-tests branch November 14, 2022 11:26
@nastra nastra reopened this Nov 14, 2022
@nastra nastra closed this Nov 14, 2022
@nastra nastra deleted the nessie-tests branch November 14, 2022 11:26
@nastra nastra restored the nessie-tests branch November 14, 2022 11:27
@nastra nastra reopened this Nov 14, 2022
@nastra nastra closed this Nov 14, 2022
@nastra nastra reopened this Nov 14, 2022
@nastra nastra closed this Nov 15, 2022
@nastra nastra reopened this Nov 15, 2022
@nastra
Copy link
Contributor Author

nastra commented Nov 15, 2022

@beinan any idea why a different and unrelated CI action fails randomly?

@ChunxuTang
Copy link
Member

@nastra Let me rerun the tests and see whether they could pass this time.

@nastra
Copy link
Contributor Author

nastra commented Nov 24, 2022

@ChunxuTang looks like CI is passing, is this good to be merged then?

ChunxuTang
ChunxuTang approved these changes Nov 27, 2022
View reviewed changes
Copy link
Member

@ChunxuTang ChunxuTang left a comment

Choose a reason for hiding this comment

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

LGTM.

@ChunxuTang ChunxuTang merged commit 558677e into prestodb:master Nov 27, 2022
@nastra nastra deleted the nessie-tests branch November 28, 2022 07:40
@wanglinsong wanglinsong mentioned this pull request Jan 12, 2023
Merged
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajantha-bhat ajantha-bhat ajantha-bhat approved these changes

@beinan beinan beinan approved these changes

@ChunxuTang ChunxuTang ChunxuTang approved these changes

@dimas-b dimas-b dimas-b approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nastra @ajantha-bhat @rohanpednekar @ChunxuTang @beinan @dimas-b