Topic recovery download with capped size. Download not more than retention.policy #6797
Conversation
mmedenjak
added
kind/bug
| @@ -364,7 +364,7 @@ partition_downloader::download_log_with_capped_size( | |||
| model::offset_delta start_delta{0}; | |||
| for (auto it = offset_map.rbegin(); it != offset_map.rend(); it++) { | |||
| const auto& meta = it->second.meta; | |||
| if (total_size > max_size) { | |||
| if (total_size + meta.size_bytes > max_size) { | |||
There was a problem hiding this comment.
This probably needs a special case to download at least one segment. Otherwise if someone has e.g. 1GB segments, but sets retention to 100MB, then they will recover nothing.
There was a problem hiding this comment.
Yeah, you are right
tests/rptest/utils/si_utils.py
Outdated
| """ | ||
| size_bytes_per_ntp = {} | ||
| segments_sizes_per_ntp = {} | ||
| for _, data in chk.items(): |
There was a problem hiding this comment.
This is hard to follow, names like data and chk are quite obscure
There was a problem hiding this comment.
As well as nicer names, you could declare types in the function definition (chk: SometType, retention_policy: SomeType) to help the reader understand
Initially Shadow Indexing downloaded segments more or equal to retention.policy But in Disk log implementation, GC removes segments until total size will be less or equal to retention.policy So downloading segments to size more than retention.policy is unnecessary because they will be deleted by GC This change makes CI to download less to retention.policy
tests/rptest/utils/si_utils.py
Outdated
| """ | ||
| size_bytes_per_ntp = {} | ||
| segments_sizes_per_ntp = {} | ||
| for _node, node_segments_reports in nodes_segments_report.items(): |
There was a problem hiding this comment.
node_segments_report shadows the parameter and should probably be renamed
There was a problem hiding this comment.
parameter has data of multiple nodes, so its name is
nodes_segments_report
There was a problem hiding this comment.
Maybe for _node, report in nodes_segments_report.items()
Even if the variables don't literally shadow each other, it's a bit too subtle to have them differ by just one s in the middle.
Now tests checks that SI recover not more than retention.policy size
Cover letter
In Shadow Indexing we have option to recover size more or equal to retention.bytes . So Shadow Indexing would download segments until sum of their sizes become more or equal to retention.bytes property. (partition_recovery_manager.cc download_log_with_capped_size)
In Disk log GC we start to delete segments if their total size more than retention.bytes . So after GC we would have total size less or equal to retention.bytes . (disk_log_impl.cc size_based_gc_max_offset)
So when they are working together we have such behavior: SI downloads segments more than retention.bytes then Disk log GC removes one segment because total size more than retention.bytes .
It turned out in TopicRecoveryTest.test_size_based_retention. SI downloads segments, then segments are automatically deleted by Disk log GC, then we check that SI downloaded more than retention.bytes and test fails (because segment was deleted).
Fixes #4887
Backport Required