-
Notifications
You must be signed in to change notification settings - Fork 763
storage: admit tiered_cloud partitions to space management #30993
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
base: dev
Are you sure you want to change the base?
Changes from all commits
153100e
cdca56e
a5d1f76
717967d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1228,9 +1228,9 @@ disk_log_impl::maybe_apply_local_storage_overrides(gc_config cfg) const { | |
|
|
||
| // cloud_retention is disabled, do not override. Cloud-topic partitions | ||
| // (storage.mode in {cloud, tiered_cloud}) bypass this gate: | ||
| // is_cloud_retention_active() is false for them, but ctp_stm still needs | ||
| // is_archival_active() is false for them, but ctp_stm still needs | ||
| // the local-target override to engage under retention_local_strict. | ||
| if (!is_cloud_retention_active() && !config().cloud_topic_enabled()) { | ||
| if (!is_archival_active() && !config().cloud_topic_enabled()) { | ||
| return cfg; | ||
| } | ||
|
|
||
|
|
@@ -1310,11 +1310,15 @@ gc_config disk_log_impl::apply_local_storage_overrides(gc_config cfg) const { | |
| return cfg; | ||
| } | ||
|
|
||
| bool disk_log_impl::is_cloud_retention_active() const { | ||
| bool disk_log_impl::is_archival_active() const { | ||
| return config::shard_local_cfg().cloud_storage_enabled() | ||
| && (config().is_archival_enabled()); | ||
| } | ||
|
|
||
| bool disk_log_impl::is_cloud_gc_active() const { | ||
| return is_archival_active() || config().is_tiered_cloud(); | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The funny thing is that I had this in my feature branch - https://github.com/redpanda-data/redpanda/pull/30512/changes#diff-8caa0c9719de25bd209fa674959d9327cd55237de2fb4683761acf9bcc6de217R1314 |
||
| /* | ||
| * applies overrides for non-cloud storage settings | ||
| */ | ||
|
|
@@ -1358,6 +1362,11 @@ gc_config disk_log_impl::apply_overrides(gc_config defaults) const { | |
| } | ||
|
|
||
| ss::future<> disk_log_impl::housekeeping(housekeeping_config cfg) { | ||
| // Cloud topics do local retention/compaction elsewhere (ctp_stm trims the | ||
| // local log; compaction is at L1), so this loop does nothing for them. | ||
| if (config().cloud_topic_enabled()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need an additional check here? |
||
| co_return; | ||
| } | ||
| ss::gate::holder holder{_compaction_housekeeping_gate}; | ||
| vlog( | ||
| gclog.trace, | ||
|
|
@@ -1763,12 +1772,21 @@ ss::future<> disk_log_impl::rewrite_segment_with_offset_map( | |
| } | ||
|
|
||
| ss::future<> disk_log_impl::gc(gc_config cfg) { | ||
| // Cloud topics do local retention/compaction elsewhere (ctp_stm trims the | ||
| // local log; compaction is at L1), so disk_log_impl gc does not apply. | ||
| if (config().cloud_topic_enabled()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto, it should be bypassed on a different level
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| co_return; | ||
| } | ||
| ss::gate::holder holder{_compaction_housekeeping_gate}; | ||
| co_await do_gc(cfg); | ||
| } | ||
|
|
||
| ss::future<std::optional<model::offset>> disk_log_impl::do_gc(gc_config cfg) { | ||
| vassert(!_closed, "gc on closed log - {}", *this); | ||
| vassert( | ||
| !config().cloud_topic_enabled(), | ||
| "[{}] gc on cloud topic partition", | ||
| config().ntp()); | ||
|
oleiman marked this conversation as resolved.
|
||
|
|
||
| cfg = apply_overrides(cfg); | ||
|
|
||
|
|
@@ -1783,7 +1801,7 @@ ss::future<std::optional<model::offset>> disk_log_impl::do_gc(gc_config cfg) { | |
| const auto offset = _cloud_gc_offset.value(); | ||
| _cloud_gc_offset.reset(); | ||
|
|
||
| if (!is_cloud_retention_active()) { | ||
| if (!is_archival_active()) { | ||
| vlog( | ||
| gclog.warn, | ||
| "[{}] expected remote retention to be active", | ||
|
|
@@ -1916,12 +1934,12 @@ disk_log_impl::compute_gc_offset(gc_config cfg) { | |
| // ctp_stm for cloud-topic partitions. The offset is retention-driven | ||
| // unless space management has pinned _cloud_gc_offset, which then takes | ||
| // precedence. maybe_apply_local_storage_overrides | ||
| // bypasses the is_cloud_retention_active() gate for cloud-topic partitions | ||
| // bypasses the is_archival_active() gate for cloud-topic partitions | ||
| // (the gate is false for storage.mode in {cloud, tiered_cloud}), so the | ||
| // local-target override engages there under retention_local_strict. | ||
| cfg = apply_kafka_retention_overrides(cfg); | ||
| if (_cloud_gc_offset.has_value()) { | ||
| co_return _cloud_gc_offset; | ||
| co_return std::exchange(_cloud_gc_offset, std::nullopt); | ||
|
oleiman marked this conversation as resolved.
|
||
| } | ||
| co_await maybe_adjust_retention_timestamps(); | ||
| cfg = maybe_apply_local_storage_overrides(cfg); | ||
|
|
@@ -3921,7 +3939,7 @@ disk_log_impl::disk_usage_and_reclaimable_space(gc_config input_cfg) { | |
| && seg->offsets().get_dirty_offset() <= retention_offset.value()) { | ||
| retention_segments.push_back(seg); | ||
| } else if ( | ||
| is_cloud_retention_active() | ||
| is_cloud_gc_active() | ||
| && seg->offsets().get_dirty_offset() <= max_removable) { | ||
| available_segments.push_back(seg); | ||
| } else { | ||
|
|
@@ -3937,8 +3955,8 @@ disk_log_impl::disk_usage_and_reclaimable_space(gc_config input_cfg) { | |
| * get_reclaimable_offsets is going to be merged together. | ||
| */ | ||
| if ( | ||
| !config().is_read_replica_mode_enabled() | ||
| && is_cloud_retention_active() && seg != _segs.back() | ||
| !config().is_read_replica_mode_enabled() && is_cloud_gc_active() | ||
| && seg != _segs.back() | ||
| && seg->offsets().get_dirty_offset() <= max_removable | ||
| && local_retention_offset.has_value() | ||
| && seg->offsets().get_dirty_offset() | ||
|
|
@@ -4279,8 +4297,8 @@ ss::future<usage_report> disk_log_impl::disk_usage(gc_config cfg) { | |
| chunked_vector<ss::lw_shared_ptr<segment>> | ||
| disk_log_impl::cloud_gc_eligible_segments() { | ||
| vassert( | ||
| is_cloud_retention_active(), | ||
| "Expected {} to have cloud retention enabled", | ||
| is_cloud_gc_active(), | ||
| "Expected cloud GC to be active for {}", | ||
| config().ntp()); | ||
|
oleiman marked this conversation as resolved.
|
||
|
|
||
| constexpr size_t keep_segs = 1; | ||
|
|
@@ -4313,7 +4331,7 @@ disk_log_impl::cloud_gc_eligible_segments() { | |
| } | ||
|
|
||
| void disk_log_impl::set_cloud_gc_offset(model::offset offset) { | ||
| if (!is_cloud_retention_active()) { | ||
| if (!is_cloud_gc_active()) { | ||
| vlog( | ||
| stlog.debug, | ||
| "Ignoring request to set GC offset on non-cloud enabled partition " | ||
|
|
@@ -4338,7 +4356,7 @@ disk_log_impl::get_reclaimable_offsets(gc_config cfg) { | |
|
|
||
| reclaimable_offsets res; | ||
|
|
||
| if (!is_cloud_retention_active()) { | ||
| if (!is_cloud_gc_active()) { | ||
| vlog( | ||
| stlog.debug, | ||
| "Reporting no reclaimable offsets for non-cloud partition {}", | ||
|
|
@@ -4540,7 +4558,7 @@ size_t disk_log_impl::reclaimable_size_bytes() const { | |
| * local retention size may change. catch these before reporting potentially | ||
| * stale information. | ||
| */ | ||
| if (!is_cloud_retention_active()) { | ||
| if (!is_cloud_gc_active()) { | ||
| return 0; | ||
| } | ||
| if (config().is_read_replica_mode_enabled()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -210,9 +210,9 @@ class disk_log_impl final : public log { | |
| /// Applies retention overrides (callers need not pre-apply them) and | ||
| /// adjusts bogus (future) retention timestamps, mutating segment indexes | ||
| /// when an entire segment is bogus, then returns the offset GC would | ||
| /// evict to -- usually derived from local retention, but when space | ||
| /// management has set _cloud_gc_offset that offset is returned instead | ||
| /// (without resetting it -- that is do_gc's responsibility). | ||
| /// evict to. Usually derived from local retention; when space management | ||
| /// has set _cloud_gc_offset, that offset is returned and consumed (reset) | ||
| /// here, so the caller is responsible for acting on it. | ||
| ss::future<std::optional<model::offset>> | ||
| compute_gc_offset(gc_config cfg) final; | ||
|
|
||
|
|
@@ -421,7 +421,11 @@ class disk_log_impl final : public log { | |
| gc_config maybe_apply_local_storage_overrides(gc_config) const; | ||
| gc_config apply_local_storage_overrides(gc_config) const; | ||
|
|
||
| bool is_cloud_retention_active() const; | ||
| bool is_archival_active() const; | ||
| // True when local segments are a reclaimable cache of cloud-resident data | ||
| // (legacy tiered storage or tiered_cloud); broader than | ||
| // is_archival_active(), which is archival-only. | ||
| bool is_cloud_gc_active() const; | ||
|
Comment on lines
+425
to
+428
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: IMO the naming of these don't make it clear that there's any difference between the two. Would it make sense to make is_cloud_retention_active refer to tiered storage specifically or something? Or if the idea is that the behavior changes because of local housekeeping, is there be a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah fair
like replace cloud_retention_active with that, yeah? and leave cloud_gc_active alone? i don't mind that. ctp housekeeping loop is also cleaning up local data, but local_housekeeping as the condition governing the disk log impl code reads a bit better IMO.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renamed is_cloud_retention_active to is_archival_active, which is at least a bit more accurate to its semantics. left is_cloud_gc_active alone. idk. this is all sort of circuitous but I can't think of a better factoring right now. |
||
|
|
||
| // returns retention_offset(cfg) but may also first apply adjustments to | ||
| // future timestamps if this option is turned on in configuration. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
&& !is_tieredhere, everytiered_cloudpartition now falls into the boundedwait(retry_backoff_time)branch unconditionally, so an idle partition wakes and re-runscompute_local_retention_offset()every 5s forever (previously it could block untimed until_lro_advanced). Thesnapshot_and_truncate_logcall is a cheap no-op when the target hasn't advanced (it early-returns on_last_snapshot_index >= eviction_point), so this is mostly the periodic recompute cost — but on a broker hosting many tiered_cloud partitions it's a non-trivial number of steady-state wakeups.The root cause is that
disk_space_managersets the pin out of band without signalling_lro_advanced. A cleaner long-term fix would be for the eviction-policy install path /set_cloud_gc_offsetto signal the stm so it could keep the untimed wait. Not blocking — worth a follow-up note.