adapter: Add coordinator consistency check #21740
Merged
Conversation
This file contains hidden or 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
jkosh44
approved these changes
Sep 13, 2023
def-
reviewed
Sep 13, 2023
There was a problem hiding this comment.
I triggered nightly: https://buildkite.com/materialize/nightlies/builds/4008
|
Whole bunch of failures and panics in nightly, seems related to this change: https://buildkite.com/materialize/nightlies/builds/4009 |
ParkMyCar
approved these changes
Sep 14, 2023
src/testdrive/src/action/sql.rs
Outdated
| state.materialize_internal_http_addr, | ||
| )) | ||
| .await? | ||
| .text() |
There was a problem hiding this comment.
You should be able to call .json() here which will parse the body as JSON
There was a problem hiding this comment.
Ah thanks for that. I was thinking about leaving it as is b/c i've added more context when each step can fail which has been helpful while debugging
def-
previously requested changes
Sep 14, 2023
jkosh44
reviewed
Sep 16, 2023
jkosh44
reviewed
Sep 19, 2023
src/adapter/src/coord/ddl.rs
Outdated
Comment on lines
446
to
455
| for id in self | ||
| .catalog() | ||
| .get_cluster(cluster_id) | ||
| .log_indexes | ||
| .values() | ||
| .cloned() | ||
| .collect_vec() | ||
| { | ||
| self.drop_compute_read_policy(&id); | ||
| } |
There was a problem hiding this comment.
Two notes:
- I would move this to before we drop the cluster. It's possible that
drop_clusterwipes out some metadata needed to update the read policies of the log indexes. - You'll probably need to collect the log indexes before executing the catalog transaction. Since the transaction completed and this cluster was dropped, the cluster won't exist in the catalog.
|
This PR has higher risk. Make sure to carefully review the file hotspots. In addition to having this change reviewed, adequate tests should be considered and it may be useful to add observability and/or a feature flag. What's This?
Buggy File Hotspots:
|
@def- i think we're good now |
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.
Motivation
Piggybacking on #21717, adding consistency checks to the coordinator that get run in dev and during testdrive
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.