Skip to content

storage: when envd reconnects, report current status at latest timestamp#31535

Merged
aljoscha merged 1 commit intoMaterializeInc:mainfrom
aljoscha:storage-report-initial-status-at-now
Feb 19, 2025
Merged

storage: when envd reconnects, report current status at latest timestamp#31535
aljoscha merged 1 commit intoMaterializeInc:mainfrom
aljoscha:storage-report-initial-status-at-now

Conversation

@aljoscha
Copy link
Contributor

This makes it so that status updates from the latest deployment, for example when coming out of read-only mode, sort last in mz_source/sink_status_history, so get surfaced as the latest status in mz_source/sink_statuses.

This fixes (or makes less likely) a bug where a failing/stalled source/sink in the old deployment would crowd out the running status of objects in the new deployment.

This fix work, but doesn't feel ideal. A problem is that the raw status collections are append-only collections, so envd will simply append status updates to the end. One could argue that the smarts should be in envd/the controller. But ultimately, it would also have to append a status update that "trumps" the earlier updates, so would do the same thing that we're achieving with this change.

Fixes MaterializeInc/database-issues#8863

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.

@aljoscha aljoscha requested review from def- and teskje February 18, 2025 16:28
@aljoscha aljoscha requested a review from a team as a code owner February 18, 2025 16:28
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 for adding a test!

Copy link
Contributor

@teskje teskje left a comment

Choose a reason for hiding this comment

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

Seems fine! I agree it's not ideal with the reliance on clock synchronization between different clusterd processes.

Have you considered selecting the occurred_at time in the controller, before writing down a new status? That would make sure that times are always increasing and cannot race. It would change the semantics of occurred_at from "when the cluster learned about it" to "when the controller learned about it" but does that matter?

@aljoscha
Copy link
Contributor Author

Have you considered selecting the occurred_at time in the controller, before writing down a new status? That would make sure that times are always increasing and cannot race. It would change the semantics of occurred_at from "when the cluster learned about it" to "when the controller learned about it" but does that matter?

That's a good point, but I didn't want to futz with the semantics too much, plus if we just took the "now" timestamp on envd that also wouldn't protect against clock skew. I think the proper solution would be to use the timestamp oracle, but the controller (who writes down these status updates) doesn't have access to that right now. So it's a whole long tail of things that would have to be changed. 🙈

This makes it so that status updates from the latest deployment, for
example when coming out of read-only mode, sort last in
mz_source/sink_status_history, so get surfaced as the latest status in
mz_source/sink_statuses.

This fixes (or makes less likely) a bug where a failing/stalled
source/sink in the old deployment would crowd out the running status of
objects in the new deployment.

This fix work, but doesn't feel ideal. A problem is that the raw status
collections are append-only collections, so `envd` will simply append
status updates to the end. One could argue that the smarts should be in
`envd`/the controller. But ultimately, it would also have to append a
status update that "trumps" the earlier updates, so would do the same
thing that we're achieving with this change.

Fixes MaterializeInc/database-issues#8863
@aljoscha aljoscha force-pushed the storage-report-initial-status-at-now branch from 7ef41e6 to 25101bf Compare February 18, 2025 19:16
@aljoscha aljoscha merged commit fd6e7fa into MaterializeInc:main Feb 19, 2025
78 checks passed
@aljoscha aljoscha deleted the storage-report-initial-status-at-now branch February 19, 2025 08:09
@teskje
Copy link
Contributor

teskje commented Feb 19, 2025

True... Another proper solution would be to remove the occurred_at column and use the update timestamp instead, i.e. have a retained-history relation instead of log. The catch of course being that retained-history relations are not ergonomic to use in Mz at the moment.

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.

3 participants