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.

Sign up for GitHub

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

Jump to bottom

storage: when dropping objects, eagerly clean up state and update builtins #31515

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

storage: when dropping objects, eagerly clean up state and update builtins #31515

wants to merge 7 commits into from

Conversation

aljoscha
Copy link
Contributor

@aljoscha aljoscha commented Feb 16, 2025 •
edited
Loading

NOTE: This is now a whole bunch of other cleanups that were enabled/or made feasible by this initial change to eagerly clean out state and update builtin collections

Before this change, we had a more sequentialized/blocking protocol for
dropping sources (and other collections):

  1. envd receives DDL that drops source
  2. envd releases read holds and sends AllowCompaction to clusterd
  3. clusterd drops source and sends back a DroppedId message
  4. envd cleans up it's collection state and updates builtin collections:
  • updating mz_storage_shards (the global id -> shard id mapping)
  • updating source/sink status to "dropped"
  • removing statistics tracking

Especially the transition between 3. and 4. can block a while if a
cluster is unresponsive.

Plus, in some situations we don't get DroppedId messages, so we would
not do these updates/cleanups. The common of these situations are:

  • dropping a replica right after dropping sources/sinks
  • dropping a whole cluster

Now, we eagerly apply updates to our builtin collections and also
eagerly clean up state. The protocol becomes:

  1. envd receives DDL that drops source
  2. in parallel:
    2.a: envd releases read holds and sends AllowCompaction to clusterd
    2.b. envd cleans up it's collection state and updates builtin collections:
  • updating mz_storage_shards (the global id -> shard id mapping)
  • updating source/sink status to "dropped"
  • removing statistics tracking
  1. clusterd drops source and sends back a DroppedId message

The benefits of this change:

  • less code
  • control flow of dropping things is more localized and easier to
    understand
  • we fix problems where we inadvertently didn't drop state and update
    builtin collections

@aljoscha aljoscha force-pushed the storage-eager-state-cleanup branch 7 times, most recently from 1815fa2 to 0d8923a Compare February 18, 2025 14:52
bkirwi
bkirwi reviewed Feb 18, 2025
View reviewed changes
src/storage-controller/src/lib.rs Outdated
"DroppedId for ID {id} but we have neither ingestion nor export \
under that ID"
);
let prev = self.dropped_ids.remove(&id);
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this interact with multi-replica storage clusters? Do we only expect to get one dropped id message globally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, dammit, DroppedId is a bit confusing, because for some things I thing the replica client logic only forwards a thing when all of the replicas agree, but I think you're right and for DroppedId they pass through as is, so we get multiple of them. I would just not remove things from dropped_ids and accept the small cost of keeping these around until a restart. ((I don't like using dropped_ids to begin with, but it's a compromise. ))

Copy link
Contributor

Choose a reason for hiding this comment

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

I would just not remove things from dropped_ids and accept the small cost of keeping these around until a restart.

I'm not a fan of intentionally leaking memory. I think the storage controller has done that previously with the collection (and/or export) state and it turned out fine, but someone running CREATE/DROP in a loop could still oom envd. We definitely can't do it in compute where transient dataflows are created and dropped all the time.

@aljoscha aljoscha force-pushed the storage-eager-state-cleanup branch 16 times, most recently from b42116a to 84c211b Compare February 20, 2025 19:36
@aljoscha aljoscha changed the title storage: when dropping objects, eagerly clean up state and update collections storage: when dropping objects, eagerly clean up state and update builtins Feb 20, 2025
@aljoscha
storage: eagerly drop source/collection state in drop_sources_unvalid…
e9158cd 
…ated

Before this change, we had a more sequentialized/blocking protocol for
dropping sources (and other collections):

1. envd receives DDL that drops source
2. envd releases read holds and sends AllowCompaction to clusterd
3. clusterd drops source and sends back a DroppedId message
4. envd cleans up it's collection state and updates builtin collections:
 - updating mz_storage_shards (the global id -> shard id mapping)
 - updating source/sink status to "dropped"
 - removing statistics tracking

Especially the transition between 3. and 4. can block a while if a
cluster is unresponsive.

Plus, in some situations we don't get DroppedId messages, so we would
not do these updates/cleanups. The common of these situations are:
- dropping a replica right after dropping sources/sinks
- dropping a whole cluster

Now, we eagerly apply updates to our builtin collections and also
eagerly clean up state. The protocol becomes:

1. envd receives DDL that drops source
2. in parallel:
 2.a: envd releases read holds and sends AllowCompaction to clusterd
 2.b. envd cleans up it's collection state and updates builtin collections:
  - updating mz_storage_shards (the global id -> shard id mapping)
  - updating source/sink status to "dropped"
  - removing statistics tracking
3. clusterd drops source and sends back a DroppedId message

The benefits of this change:
- less code
- control flow of dropping things is more localized and easier to
  understand
- we fix problems where we inadvertently didn't drop state and update
  builtin collections
@aljoscha
storage: proper cleanup and state update for progress collections
5fee2e0
@aljoscha
storage: eagerly drop sink state in drop_sources_unvalidated
51a7ff1 
Same commit as the previous one, but for sinks.

Before, we were waiting on DroppedId messages for a couple of of things:
- cleaning up collection state in the controller
- updating mz_storage_shards (the global id -> shard id mapping)
- updating source/sink status to "dropped"
- removing statistics tracking

In some situations we don't get DroppedId messages, so we would not do
these updates/cleanups. The common of these situations are:
- dropping a replica right after dropping sources/sinks
- dropping a whole cluster

Now, we eagerly apply updates to our internal collections and also
eagerly clean up state.
@aljoscha aljoscha force-pushed the storage-eager-state-cleanup branch from 84c211b to cd29c07 Compare February 21, 2025 10:48
@aljoscha
storage: remove PendingCompactionCommand, send commands in update_hol…
8505476 
…d_capabilities

This is a remnant from a moment where acquiring an instance client
required async. But we don't have that anymore so we can simplify
`process()` big time.
@aljoscha aljoscha force-pushed the storage-eager-state-cleanup branch from cd29c07 to 0bf850d Compare February 21, 2025 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkirwi bkirwi bkirwi left review comments

@teskje teskje teskje left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aljoscha @bkirwi @teskje