transform: gc committed offsets #16526
Merged
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
rockwotj
force-pushed
the
gc-committed-offsets
branch
from
ea93e12
to
31343d0
Compare
rockwotj
force-pushed
the
gc-committed-offsets
branch
from
31343d0
to
297864d
Compare
|
new failures in https://buildkite.com/redpanda/redpanda/builds/44903#018d8a18-b4e3-4667-bc3f-e7586c03af85: |
bharathv
previously approved these changes
Feb 20, 2024
src/v/cluster/distributed_kv_stm.h
Outdated
|
|
||
| ss::future<errc> remove_all(ss::noncopyable_function<bool(Key)> pred) { | ||
| auto holder = _gate.hold(); | ||
| auto units = co_await _snapshot_lock.hold_read_lock(); |
There was a problem hiding this comment.
I feel like this could hold a write lock since remove_all seems like a rare operation and the side effects of it are easier to explain if the entire operation sees a consistent snapshot of the state.
There was a problem hiding this comment.
Yeah it is a rare case, so probably cleaner to make it see consistent state. I've pushed an update.
rockwotj
force-pushed
the
gc-committed-offsets
branch
from
297864d
to
8776bf6
Compare
bharathv
reviewed
Mar 4, 2024
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Based on a set transform IDs, this will be used to GC deleted transforms. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
This will be used to GC offsets when transforms are deleted. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
note the comments in the transform::service::garbage_collect_committed_offsets about the consistency of these operations. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
With all the caveats mentioned in the `transform::service` documentation Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
This makes sure our debug endpoint to cleanup offsets works. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
In accordance with new documentation standards document some more methods in this stm. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
rockwotj
force-pushed
the
gc-committed-offsets
branch
from
8776bf6
to
cfef68c
Compare
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45625#018e0a7c-58db-4ee6-ab18-aceb9539c21d |
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.
Prior to this patch set - it was not possible to cleanup committed offsets. This means that if lots of transforms were created and then deleted one could run into the limit with no escape hatch. This patch set implements an escape hatch to cleanup offsets.
This is implemented as a manual GC mechanism because this can race with deploys and delete offsets that are still in use. A way to get around that is to expose a max transform id or something from the controller log so that we don't ever delete any ids that the target shard has not yet seen. Since IDs are assigned from the first deploy and their offset within the controller log, we could do this by the committed offset of the controller log. However, instead of implementing that and running it periodically, we just make it a manual endpoint to hit, the automatic GC could happen at a later date.
This is a followup to: #16185
Backports Required
Release Notes