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

storage: write status updates in the controller, and add the "paused" status #23222

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

guswynn
Copy link
Contributor

@guswynn guswynn commented Nov 15, 2023

Implements #19792

@rkrishn7 thank you for you work on #20735 ! I have rebased and adapted that code here.


This pr shifts where we write status updates for sources and sinks into the storage controller; this reduces contention on those history shards, but also allows us to easily mark sources and sinks as "paused" when there exists no replica (note that we could have done this in a more hacky way, but chose not to)

this pr attempts to make the paused status as understandable as possible, but some edge cases will be improved when #23013 is implemented.

Motivation

  • This PR adds a known-desirable feature.

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:

@guswynn guswynn force-pushed the status-writes-in-controller branch 2 times, most recently from c216f05 to b8f148d Compare November 16, 2023 00:09
@guswynn guswynn changed the title storage: writes status updates in the controller storage: writes status updates in the controller, and add the "paused" status Nov 16, 2023
@guswynn guswynn marked this pull request as ready for review November 16, 2023 00:10
@guswynn guswynn requested a review from a team November 16, 2023 00:10
Copy link

shepherdlybot bot commented Nov 16, 2023

This PR has higher risk. Make sure to carefully review the file hotspots. In addition to having a knowledgeable reviewer, it may be useful to add observability and/or a feature flag. What's This?

Risk Score Probability Buggy File Hotspots
🔴 82 / 100 63% 2
Buggy File Hotspots:
File Percentile
../src/storage_state.rs 98
../src/lib.rs 93

@guswynn guswynn force-pushed the status-writes-in-controller branch 2 times, most recently from 436a008 to de6cfae Compare November 16, 2023 00:27
src/storage-client/src/client.rs Outdated Show resolved Hide resolved
src/storage-client/src/client.rs Outdated Show resolved Hide resolved
src/storage-client/src/client.rs Outdated Show resolved Hide resolved
src/storage-client/src/client.rs Outdated Show resolved Hide resolved
src/storage-controller/src/healthcheck.rs Outdated Show resolved Hide resolved
src/storage-controller/src/healthcheck.rs Outdated Show resolved Hide resolved
src/storage-controller/src/lib.rs Outdated Show resolved Hide resolved
src/storage-controller/src/lib.rs Outdated Show resolved Hide resolved
src/storage/src/healthcheck.rs Outdated Show resolved Hide resolved
src/storage/src/healthcheck.rs Show resolved Hide resolved
@nrainer-materialize
Copy link
Contributor

Test coverage looks good (build: https://buildkite.com/materialize/coverage/builds/317).

@guswynn guswynn force-pushed the status-writes-in-controller branch 2 times, most recently from a1609ba to 24ef729 Compare November 21, 2023 21:25
@guswynn guswynn changed the title storage: writes status updates in the controller, and add the "paused" status storage: write status updates in the controller, and add the "paused" status Nov 21, 2023
@guswynn guswynn requested review from a team, bkirwi and moulimukherjee November 21, 2023 23:30
Copy link
Contributor

@petrosagg petrosagg left a comment

Choose a reason for hiding this comment

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

Looks much better! A few more comments but that's almost ready to merge

src/storage-client/src/client.rs Show resolved Hide resolved
src/storage-controller/src/lib.rs Outdated Show resolved Hide resolved
src/storage-controller/src/rehydration.rs Outdated Show resolved Hide resolved
src/storage/src/healthcheck.rs Outdated Show resolved Hide resolved
@guswynn
Copy link
Contributor Author

guswynn commented Nov 22, 2023

@petrosagg I think I resolved the last of your comments!

Copy link
Contributor

@petrosagg petrosagg left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@guswynn guswynn enabled auto-merge (rebase) November 27, 2023 16:27
@guswynn guswynn merged commit 0716439 into MaterializeInc:main Nov 27, 2023
75 of 76 checks passed
@guswynn guswynn deleted the status-writes-in-controller branch November 27, 2023 18:08
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

4 participants