cloud_storage: limit reader concurrency to avoid bad_allocs under random read loads #7042
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
jcsp
changed the title
jcsp
changed the title
jcsp
force-pushed
the
issue-6111-global-materialized-state
branch
from
6dc48ac
to
58cd09c
Compare
5 tasks
jcsp
force-pushed
the
issue-6111-global-materialized-state
branch
from
58cd09c
to
91d85df
Compare
mmedenjak
added
kind/bug
jcsp
force-pushed
the
issue-6111-global-materialized-state
branch
from
91d85df
to
249e8da
Compare
jcsp
force-pushed
the
issue-6111-global-materialized-state
branch
2 times, most recently
from
394042c
to
c16fb12
Compare
jcsp
commented
Nov 2, 2022
jcsp
force-pushed
the
issue-6111-global-materialized-state
branch
from
c16fb12
to
cc49769
Compare
6 tasks
jcsp
force-pushed
the
issue-6111-global-materialized-state
branch
from
cc49769
to
0c21989
Compare
These will no longer be internal to remote_partition, once we start managing the population of materialized_segment_state objects (and their associated readers) from a central place to improve resource management.
…state This is necessary to be able to evict the segment and/or its readers from outside of rememote partition: we need to know which remote_partition::_segments to update.
jcsp
force-pushed
the
issue-6111-global-materialized-state
branch
from
0c21989
to
eeedc3e
Compare
This object lives inside `remote` which is passed around to all the right places as the "api" object: we may later refactor remote into a true api wrapper and move the put/get methods down to some sub-object that is identifiably a remote storage client.
This was previously storage::adjustable_allowance, used in storage_resources for concurrency limits that could be adjusted at runtime via configuration bindings. Renaming for clarity (as the noun 'allowance' didn't really say anything that 'semaphore' doesn't say more clearly). This will also be used in cloud_storage for similar purposese.
The default reader limit is rather generous, and can easily overwhelm the RAM on a GB-per-core test configuration (less than 1000 readers is enough).
…_busy Now that we limit the reader concurrency, this should not bad_alloc any more.
Previously this was being instantiated and stopped, but never started. That results in segment/reader eviction not happening, now that `remote` contains the `materialized_segments` state.
jcsp
force-pushed
the
issue-6111-global-materialized-state
branch
from
eeedc3e
to
96f9d5c
Compare
|
Push: update the commit that updates unit tests, to cover the unit tests added in the rebase. |
Lazin
approved these changes
Nov 4, 2022
| auto deadline = st.atime + max_idle; | ||
| if (now >= deadline && !st.segment->download_in_progress()) { | ||
| if (st.segment.owned()) { | ||
| vlog( |
There was a problem hiding this comment.
the log line shuld have an ntp because many partitions could use the same GC loop
There was a problem hiding this comment.
Right, I'll add it in the followup PR.
| // be disposed before the remote_partition it points to. | ||
| vassert( | ||
| false, | ||
| "materialized_segment_state outlived remote_partition"); |
There was a problem hiding this comment.
In this path we unfortunately can't, because the parent pointer is needed to fetch the ntp
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.
Cover letter
Previously, there was no limit on how many remote_segment_batch_reader objects might be instantiated at the same time. These are comparatively heavyweight objects, containing:
Readers are re-used when a client consumes a contiguous range (i.e. next fetch picks up the reader from the previous fetch and continues it), but clients can also do random reads, picking offsets and issuing a single fetch there. In the random read case, the population of readers could grow to the limit of one per offset. Collection of stale readers only happens when a segment is hydrated (which is never if all the segments are already hydrated), or after a segment's TTL is exceeded (which is also never if there are continuous fetch requests that keep touching the segment atime).
For a more robust bound on the number of readers that exist at any one time, the tracking of
materialized_segment_stateis moved from each individualremote_partitioninto a newmaterialized_segmentsobject that is responsible for managing all the temporary read state for partitions. Readers themselves now carry semaphore units that belong to a central semaphore which has an initial size set by a new config property,cloud_storage_max_readers_per_shard.Fixes #6111
Fixes #6023
Backport Required
If it becomes necessary for someone hitting this in the field, we might backport to 22.2, but otherwise we shouldn't: it's a rather big change for a backport.
UX changes
None
Release notes
Improvements
cloud_storage_max_readers_per_shardis added, which controls the maximum number of cloud storage reader objects that may exist per CPU core: this may be tuned downward to reduce memory consumption at the possible cost of read throughput. The default setting one per partition (i.e. the value oftopic_partitions_per_shardis used).