[v22.3.x] cloud_storage: move eviction under remote_partition #9933
Conversation
andrwng
changed the title
|
@andrwng anything blocking triage of the failures, making this ready for review, and assigning reviewers? |
|
I need to spend a bit more time on this. The failures I'm seeing in CI indicate a real race/unsafe memory access with shutdown. |
CONFLICT: - required adding an abort source to remote_partition Previously each remote_partition would wait for an eviction barrier to pass through the eviction loop, ensuring all segments are destructed before stopping the partition. Each segment references members of the remote_partition, so it's important the shutdown sequence stops the segments before destructing the remote_partition. At the same time, having each partition wait for another set of partitions to finish flushing can result in a slow shutdown. This commit moves the eviction loop into the remote_partition, allowing partition shutdown to entirely avoid waiting for any other partition to shut down, while still ensuring that each underlying segment is destructed after the remote_partition. Without this commit, I witnessed the period of partition shutdown in a heavily loaded server take 30 minutes. With this commit I see a similarly shaped shutdown taking 10 seconds. Related redpanda-data#9569 (cherry picked from commit 03587d8)
...and use it in remote_partition::erase. This is necessary because we now require a usable abourt source in all cloud storage paths, and the partition's abort source is already fired once we get to removing the persistent state.
andrwng
force-pushed
the
v22.3.x-evict-from-partition
branch
from
9de1010
to
c09f0c7
Compare
|
I don't see anything wrong with the code, but the |
|
/ci-repeat 3 |
| @@ -186,11 +192,40 @@ class remote_partition | |||
| retry_chain_node _rtc; | |||
| retry_chain_logger _ctxlog; | |||
| ss::gate _gate; | |||
| ss::abort_source _as; | |||
There was a problem hiding this comment.
Would it not have been possible to backport 632676c instead of adding the abort source manually?
There was a problem hiding this comment.
Yeah that would have been possible. At the time I hadn't considered cherry-picking individual commits from the backport, but that is a better approach
|
The CI failures were because #10342 was missing. I triggered a rebuild to rebase, now that that's merged. |
Backport of #9590
CONFLICT:
Previously each remote_partition would wait for an eviction barrier to pass through the eviction loop, ensuring all segments are destructed before stopping the partition. Each segment references members of the remote_partition, so it's important the shutdown sequence stops the segments before destructing the remote_partition. At the same time, having each partition wait for another set of partitions to finish flushing can result in a slow shutdown.
This commit moves the eviction loop into the remote_partition, allowing partition shutdown to entirely avoid waiting for any other partition to shut down, while still ensuring that each underlying segment is destructed after the remote_partition.
Without this commit, I witnessed the period of partition shutdown in a heavily loaded server take 30 minutes. With this commit I see a similarly shaped shutdown taking 10 seconds.
Related #9569
(cherry picked from commit 03587d8)
Backports Required
Release Notes
Improvements