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

slt: Fix/disable broken explain plan #21771

Merged
merged 1 commit into from
Sep 15, 2023
Merged

Conversation

def-
Copy link
Contributor

@def- def- commented Sep 15, 2023

Came in via #21681

Explain plan changed inbetween to contain further information. I'll verify locally that this fixes it. Edit: Verified.

Test run: https://buildkite.com/materialize/tests/builds/63827#018a99b7-2ee9-48ec-9030-ce8a578dff26

Motivation

  • This PR fixes a previously unreported bug.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:

@def- def- changed the title slt: Fix broken explain plan slt: Fix broken explain plan merge skew Sep 15, 2023
@def-
Copy link
Contributor Author

def- commented Sep 15, 2023

Even worse, could this be flaky? Can we make sure the explain plan always returns the same result?

Edit: This is not merge skew, but a flaky explain. I'm not sure if we can make the explain deterministic, will remove it for now.

@def- def- force-pushed the pr-slt-fix branch 2 times, most recently from 9221c0d to baa9930 Compare September 15, 2023 18:16
@mgree
Copy link
Contributor

mgree commented Sep 15, 2023

This test could definitely be flaky, as it's about timeouts. If it's breaking things, you (or I---whatever's easiest) can pull out the offending tests in cardinality.slt.

@def- def- changed the title slt: Fix broken explain plan merge skew slt: Fix broken explain plan Sep 15, 2023
…egg-timeouts

also retry timeouts when talking to frontegg
@def-
Copy link
Contributor Author

def- commented Sep 15, 2023

@mgree If you stamp this PR it should auto-merge in a few minutes and disable the test for now.

@def- def- merged commit 7e2b1ee into MaterializeInc:main Sep 15, 2023
@nrainer-materialize nrainer-materialize changed the title slt: Fix broken explain plan slt: Fix/disable broken explain plan Sep 15, 2023
@def- def- deleted the pr-slt-fix branch September 15, 2023 22:32
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.

4 participants