Skip to content

Conversation

@ggevay
Copy link
Contributor

@ggevay ggevay commented Dec 8, 2024

The soft_assert_or_log that signals when ColumnKnowledge does work after EquivalencePropagation has been unexpectedly happening a lot in CI. We have enough examples to start fixing the issue, and we don't want to keep CI flaky while we do so. Therefore, this PR downgrades this assert to just a tracing::error!. This way, it won't fail CI, but it will still emit a Sentry error when it happens on staging or production.

Additionally, the PR also:

  • Includes the dataflow's ID in the error msg.
  • Fixes plan tracing, so that we can separately observe the result of EQProp before CK happens.

Motivation

Tips for reviewer

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • 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).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

Also:
- Include the dataflow's ID in the error msg.
- Fix plan tracing, so that we can separately observe the result of
  EQProp before CK happens.
@ggevay ggevay added A-optimization Area: query optimization and transformation A-CLUSTER Topics related to the CLUSTER layer labels Dec 8, 2024
@ggevay ggevay requested review from def- and frankmcsherry December 8, 2024 12:21
@ggevay ggevay requested a review from a team as a code owner December 8, 2024 12:21
@def- def- requested a review from a team as a code owner December 8, 2024 12:35
Copy link
Contributor

@def- def- left a comment

Choose a reason for hiding this comment

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

Thanks, I removed the two ignored tests, good for me if they are green now: https://buildkite.com/materialize/nightly/builds/10649 Edit: Green

@ggevay
Copy link
Contributor Author

ggevay commented Dec 8, 2024

Thank you! Merging.

@ggevay ggevay merged commit 07bacda into MaterializeInc:main Dec 8, 2024
90 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-CLUSTER Topics related to the CLUSTER layer A-optimization Area: query optimization and transformation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants