materializations: don't persist a spec in the destination system #2262
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Updates the spec snapshot, which had apparently gotten out of date.
The prereqs checks and tests for materialize-postgres and materialize-redshift haven't worked since the upgrade to pgx v5.0 They are fixed here. It is somewhat unfortunate that the connection errors don't include the error code explicitly anymore, but string matching is still possible for most of the common cases. The materialize-snowflake "wrong account in host" check just completely doesn't work anymore, since Snowflake changed their error response from something we could parse to an almost malevolently useless "an unknown error has occurred" message. I guess we'll get to have some fun helping users figure that out. A few other general fixes to prereq tests are included here as well. Mainly the "wrong address" tests don't really work reliably when running the tests locally so they have been removed.
It is apparently possible for a job to fail immediately, which has been observed when performing a large number of table alterations in rapid succession to accomplish several table column migrations. This has the unfortunate consequence of causing calls to `job.Wait` to hang seemingly forever. So we must check for this condition and retry if it's appropriate.
…igrations} A few updates to get the `TestValidateAndApply` and `TestValidateAndApplyMigrations` tests consistently running locally for me. Mostly the `TestValidateAndApply` snapshots needed updated for the updated constraints that get returned when a column is migratable, and some updates to the migration test output to smooth out timezone stuff.
williamhbaker
commented
Jan 9, 2025
materialize-sql/driver.go
Outdated
| @@ -200,6 +189,22 @@ func (d *Driver) Apply(ctx context.Context, req *pm.Request_Apply) (*pm.Response | |||
| return nil, err | |||
| } | |||
|
|
|||
| if endpoint.MetaCheckpoints != nil && !is.HasResource(endpoint.MetaCheckpoints.Path) { | |||
There was a problem hiding this comment.
I moved the concept of a "metadata table" entirely out of the materialize-boilerplate and into materialize-sql. The checkpoints table is now created here if it is needed. A checkpoints table is only currently used by SQL materializations so it seemed to make sense as a materialize-sql thing, although I honestly don't feel really strongly about this.
williamhbaker
force-pushed
the
wb/dont-persist-specs
branch
from
65c7383 to
cbd025c
Compare
Validate and Apply request messages have had the last successfully validated/applied specification included with them for a while now. We can use this prior spec, rather than needing to store and retrieve it out of the destination metadata table. This does introduce a tiny difference in behavior, where currently Validate runs against the last successfully Applied (persisted) spec, rather than the last successfully Validate spec. I can't think of any practical difference this would make, and this model has been running successfully with other materializations already, such as Iceberg.
Updates all the test snapshots for materializations that used to persist a spec and now don't. The Apply action description has changed for them.
williamhbaker
force-pushed
the
wb/dont-persist-specs
branch
from
804c335 to
8f69546
Compare
In case the metadata schema doesn't exist, wait until after the missing schemas have been created to try to create the checkpoints table. Otherwise creation of the checkpoints table may fail if the metadata schema doesn't yet exist.
mdibaiee
approved these changes
Jan 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
Implements using the prior materialization specification from Validate and Apply requests, rather than loading it out of and persisting it into the destination system somehow. This is good since it eliminates a metadata table we have to create, and some systems have limitations on how large a spec can be. There's quite a bit of simplification overall.
The first several commits are mostly maintenance on our Validate & Apply test suite, getting them to run reliably for me on my local machine, which was important to verify that everything still works as expected with the tests after not loading the applied spec from the metadata table anymore.
See individual commit messages for some more details.
Closes #2076
Closes #1173
Workflow steps:
(How does one use this feature, and how has it changed)
Documentation links affected:
N/A
Notes for reviewers:
(anything that might help someone review this PR)
This change is