storage: various space management non-functional changes #18314
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
I found the name a bit vague in trying to understand what this code was doing. This updates it to lowest_ts_to_retain(), and updates the disk_space_manager to use it rather than duplicating the logic.
target_excess -> adjusted_target_excess: I find the name more intuitive, despite being more verbose at call sites, as a reader I prefer the reminder that the value has been tweaked
I found the existing logging a bit confusing because "tiered storage retention" isn't widely used verbiage. This tweaks it to refer to this as "space management of tiered storage topics". Also s/recover/remove: it feels a bit clearer, segment removal isn't referred to as "recovering" in other contexts.
The previous name is pretty vague: as a reader it's unclear what it's actually doing other than modifying the config in some way. Updates it to explicitly mention that it is handling local storage overrides. IMO this is easier to reason about when reading the method and its call sites.
Similar to maybe_override_retention_config(), updates override_retention_config() to be called apply_local_storage_overrides().
Renames apply_base_overrides() to apply_kafka_retention_overrides(). I think it's a bit easier to infer what the Kafka retention overrides are (i.e. the ones that are Kafka compatible: retention.ms, retention.bytes). Others considered but not taken: - total_retention - full_retention - consumable_retention - topic_retention
Pulls out a couple instances of duplicate code, in an effort to clarify naming of various overrides and adjustments done around retention.
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48854#018f5e98-3726-47db-940b-4f885b23d5ae ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48854#018f5e98-3721-43fb-ac81-db10f95182f8 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48943#018f639f-fb16-4288-941e-aaad636a3dc7 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48943#018f639f-fb11-4403-949d-c80579609e83 |
nvartolomei
previously approved these changes
May 10, 2024
With the original code, it was easy to get tripped up by the nots and ors. Tweaks the implementation and adds a comment to make this easier to read. No functional changes.
andrwng
force-pushed
the
space-management-retention-renames
branch
from
636f37c
to
038e1f7
Compare
andrwng
commented
May 10, 2024
We have seen issues where the lack of tiered storage resulted in disk filling up. The info logs didn't make it clear what was happening and it was assumed to be a bug in code, rather than a misconfiguration issue. This commit bumps a log line to info level so that it would have been clear that a specific error path wasn't being taken. Also tweaks wording to makes a stronger connection to tiered storage.
andrwng
force-pushed
the
space-management-retention-renames
branch
from
038e1f7
to
9425c18
Compare
nvartolomei
approved these changes
May 10, 2024
dotnwat
reviewed
May 10, 2024
|
CI failure: #13275 |
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.
Refactors and renames some methods, and tweaks comments and logs to clarify the space management and retention code.
There are no major functional changes in this PR.
Backports Required
Release Notes