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

v.0.99: global id migration fix #27040

Merged

Conversation

sploiselle
Copy link
Contributor

#26556 had a bug in that it did not properly determine the final dependency order of all IDs.

The bug was caused by this line, which @guswynn describes on Slack:

this is because .rev hasI: DoubleEndedIterator requirement. The iterator returned by Filter is double-ended (because its build on top of a double-ended one), so it just reverses the whole stack. basically, .rev doesnt buffer the values and then reverse them after the filter.

This means we just need to try again, but with the right approach to determining the final dependency order.

In addition, to do this, we need to allow the catalog to boot, initially, with out-of-order dependencies, which means reverting 7d348b6

I've tested this extensively locally but we'll be able to ensure it works using the cloud upgrade tests.

Motivation

This PR fixes a recognized bug.

Checklist

…tstrap"

This reverts commit 7d348b6.

In trying to release v0.99.0, we discovered that v0.98.6 contained
a bug that failed to adequately adjust all items' GlobalIds to use
GlobalId's PartialOrd implementation to express dependencies.
@sploiselle sploiselle requested a review from a team as a code owner May 10, 2024 21:52
@sploiselle sploiselle requested a review from maddyblue May 10, 2024 21:52
@sploiselle sploiselle force-pushed the v099-global-id-migration-fix branch from 49e29ee to aaa43c1 Compare May 10, 2024 22:06
@sploiselle sploiselle enabled auto-merge May 10, 2024 23:17
Copy link
Member

@ParkMyCar ParkMyCar left a comment

Choose a reason for hiding this comment

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

Only took a cursory glance, let me know if you want a closer review!

@sploiselle sploiselle merged commit 2fc8dd7 into MaterializeInc:main May 10, 2024
74 checks passed
@sploiselle sploiselle deleted the v099-global-id-migration-fix branch May 10, 2024 23:18
Copy link
Contributor

@maddyblue maddyblue left a comment

Choose a reason for hiding this comment

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

LGTM.

Should the soft assert be a not soft assert?

@sploiselle
Copy link
Contributor Author

@maddyblue Yeah––that is a good point and one I thought about for a while. One of the things I've been trying to do is avoid panicking envd unless things simply cannot work. In this case, logging the Sentry error seems like a good approach because it doesn't necessarily bring down the customer environment and––if somewhere else in the code goes, "whoa that doesn't work without the dependencies being in order," then they will panic and we have a breadcrumb to find explaining the upstream issue.

This strategy was actually working; we'd been receiving Sentry notifications about this bug. Unfortunately, we were "desensitized to the signal" because of #26966 as well as the testing team being spread pretty thin atm.

If you have a different opinion on the best strategy, I will gladly defer to whatever diff you envision––just sharing how/why I approached this as I did.

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