-
Notifications
You must be signed in to change notification settings - Fork 487
Test that statement logging marks all statements finished #34492
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
Test that statement logging marks all statements finished #34492
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have many other tests using testdrive internally, which don't benefit from the check-statement-logging. I triggered a test run to see how it would look if we enabled the --check-statement-logging in all tests:
https://buildkite.com/materialize/test/builds/113436
https://buildkite.com/materialize/nightly/builds/14501
Edit: Looks good so far!
| - ./ci/plugins/mzcompose: | ||
| composition: testdrive | ||
| args: [--default-size=1, --slow] | ||
| args: [--default-size=1, --slow, --check-statement-logging] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only in this test and not in all testdrive tests?
| #[clap(long, default_value_t = ConsistencyCheckLevel::default(), value_enum)] | ||
| consistency_checks: ConsistencyCheckLevel, | ||
| /// Whether to run statement logging consistency checks (adds a few seconds at the end of every | ||
| /// test file). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, doesn't hurt in Nightly though! Does it matter how the clusters are configured? I guess not, so maybe in just one test is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems these different Testdrive jobs differ in things that don't really matter for this consistency check, so I thought I won't slow down all of them, only the fastest one.
Thank you! I see some timeouts and other issues, but it's not immediately obvious to me whether they can be related. Do the backup tests use Testdrive? But actually, I'm a bit afraid of making other tests just slightly flaky. For example, if a test restarts the system (e.g. by killing envd), then there might be rare flakes for this statement logging check, as mentioned in the doc comment of |
|
I'm not suggesting to turn it on in all tests! I think the timeouts are related, as this makes every test slower. I just wanted a one-off run to see if anything else falls over atm. |
|
I see, ok, thank you! |
This is another preparatory PR before adding statement logging to the frontend peek sequencing (#34305). This one is just adding some new tests.
The situation is that we won't be able to avoid fixing https://github.com/MaterializeInc/database-issues/issues/7304 any longer, because this will happen more often with the frontend peek sequencing (due to awaiting more on the frontend task, and thus having more time for the future to get dropped). However, the only viable way that I can see for fixing https://github.com/MaterializeInc/database-issues/issues/7304 will make us lose some test coverage: My plan to fix it is to remove the assertion in
ExecuteContextExtra::drop, and instead make it mark the statement finished in the statement log. This means that if we have some code somewhere that forgets to mark a statement finished, then we would no longer have that assert firing.So, what this PR does is to add tests that explicitly check that all statements get into a finished state in the statement log. And then when my next PR adds statement logging to the frontend peek sequencing and removes the assertion, these tests will keep testing for the finished states. (In the current PR, the new tests only check that the
finished_atandfinished_statusfields are not null, but after the assertion is removed, we'll have a new possible state infinished_status, so I'll modify these tests to also alert on that new state.)There are 3 commits:
.tdfile that is run naked, i.e., not when Testdrive is called from Cluster tests and other things. See the doc comment ofcheck_statement_loggingfor reasoning.statement-logging.td. This is not covered by 1., because this.tdis run as a Cluster test.The new tests take a few seconds to run after every
.tdfile (due to waiting for the 5-sec buffering of writing to the statement log), so I made it configurable whether Testdrive does these checks, and made it do it only in one job of Nightly, which seemed to be the fastest one. Let me know if this is ok like this.Nightly, running just the Testdrives: https://buildkite.com/materialize/nightly/builds/14493