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

tech-debt(schema-engine-tests): Reduce vitess flakeyness #4492

Merged
merged 5 commits into from
Nov 24, 2023

Conversation

miguelff
Copy link
Contributor

@miguelff miguelff commented Nov 23, 2023

Vitess DDL is eventually consistent. When DDL is applied, there's a delay until that's reflected on vtgate. That delay is equal to the TABLET_REFRESH_INTERVAL setting in vttestserver + the time it takes for the refresh operation to complete.

What I did to fix this was:

  • Add an optional field to the DBUnderTestabstraction, called max_ddl_refresh_delay which is None for all databaes where DDL is applied atomically, and has a value for vitess of 1000 ms.
  • I provided access to that setting in SchemaPush, and in case it had a value, make the schema push operation sleep after being applied.

Effects:

  • Experiments yield that 1000 ms seems big enough to remove the flakeyness. I tried with lower wait times, such as 100, 200 and 500ms but some times one of the tests failed.
  • The wait time increased substantially for this check, from 2m to 8m. Still this is less than the test feedback from other checks, so it's acceptable.
  • In the future if the number of schema tests in vitess grew substantially, the runtime will grow by 1s times the number of schema loads. This might be unnacceptable.
  • A longer term solution would be to:
    1. Do not apply the sleep strategy on all tests, and instead run tests normally.
    2. In case the test fails retry with some sleep time, up to x (eg. 3) times.
  • I don't have the appetite to implement the long-term solution now, the reason is that connector_test macros used in schema tests require a big overhaul, which takes more time than what's now considered a priority for solving the flakeyness, compared to other team priorities.

@miguelff miguelff requested a review from a team as a code owner November 23, 2023 11:59
@miguelff miguelff requested review from jkomyno and Weakky and removed request for a team November 23, 2023 11:59
@miguelff miguelff changed the title tech-debt: Reduce vitess flakeyness tech-debt: Reduce vitess flakeyness in schema-tests Nov 23, 2023
Copy link

codspeed-hq bot commented Nov 23, 2023

CodSpeed Performance Report

Merging #4492 will not alter performance

Comparing fix-vitess-flakes (9a19e30) with main (e08be2e)

Summary

✅ 11 untouched benchmarks

@miguelff miguelff changed the title tech-debt: Reduce vitess flakeyness in schema-tests tech-debt(schema-engine-tests): Reduce vitess flakeyness Nov 23, 2023
@miguelff miguelff added this to the 5.7.0 milestone Nov 24, 2023
@miguelff miguelff self-assigned this Nov 24, 2023
@@ -182,13 +182,9 @@ fn unique_constraint_errors_in_migrations(api: TestApi) {
.send_unwrap_err()
.to_user_facing();

let expected_json = expect![[r#"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated, this test depended on source code positioning of the schema push file, as it is displayed in a backtrace. I think that's wrong and changed it.

Copy link
Contributor

@janpio janpio left a comment

Choose a reason for hiding this comment

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

✅ on the description of what you implemented

Copy link
Member

@Weakky Weakky left a comment

Choose a reason for hiding this comment

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

The button Miguel, click the button 👇🔥

image

@miguelff miguelff merged commit 831ab3d into main Nov 24, 2023
58 checks passed
@miguelff miguelff deleted the fix-vitess-flakes branch November 24, 2023 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants