kvserver: remove below-raft PreIngestDelay during SST application #98762
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
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
|
Leaving as draft since only to be merged for 23.2, still, I think this can be reviewed. |
tbg
changed the title
pav-kv
approved these changes
Mar 28, 2023
tbg
force-pushed
the
no-below-raft-ingest-throttle
branch
from
e0f19f6 to
307f5b8
Compare
All modern filesystems support `Link`, so this code is probably never hit. Move it out of the way a little bit. Epic: none Release note: None
Epic: none Release note: None
We shouldn't artificially delay SST ingestion below raft because this exacerbates memory pressure (cockroachdb#81834). Anecdotally I see in my "typical experiment" (restores under I/O bandwidth constraints) that `PreIngestDelay` is mostly fairly small compared to the slowness that comes from write bandwidth overload itself, so at least in those experiments removing this has little to no effect. As we are also working on replication admission control[^1] and are looking into improving the memory footprint under unchecked overload[^2] now's a good time to rip this out as we'll be in a good place to address any detrimental fallout from doing so. [^1]: cockroachdb#95563 [^2]: cockroachdb#98576 Touches cockroachdb#81834. Touches cockroachdb#57247. Epic: none Release note: None
tbg
force-pushed
the
no-below-raft-ingest-throttle
branch
from
307f5b8 to
d3afb8b
Compare
erikgrinaker
approved these changes
Apr 14, 2023
|
bors r=erikgrinaker |
|
This PR was included in a batch that timed out, it will be automatically retried |
|
Build succeeded: |
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Apr 26, 2023
We now rely on admission control to protect the nodes.[^1] [^1]: cockroachdb#98762 (comment) Epic: none Release note (ops change): The following two cluster settings were retired and no longer have any effect: - kv.bulk_io_write.concurrent_addsstable_requests - kv.bulk_io_write.concurrent_addsstable_as_writes_requests
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Jun 14, 2023
Fixes cockroachdb#102683. Part of cockroachdb#104154. These were added way back in cockroachdb#36403 and cockroachdb#73904, pre-dating much of IO admission control for leaseholder writes. With cockroachdb#95563, we now have IO admission control for follower writes. Put together, have ample LSM read-amp protection through AC alone. These concurrency limiters are now redundant and oblivious to more sophisticated AC measures. We recently removed the below-raft equivalents of these limiters (cockroachdb#98762), and like mentioned there, these limiters can exacerbate memory pressure. Separately, we're looking to work on speedier restores, and these limiters are starting to get in the way. While here, we also disable the pre-ingest delay mechanism in pebble, which too pre-dates AC, introduced way back in cockroachdb#34258 for RocksDB and in \cockroachdb#41839 for Pebble. IO AC is able to limit the number of L0 files, and this pre-ingest delay with its maximum per-request delay time of 5s can be less than effective. It's worth noting that the L0 file count threshold at which this pre-ingest delay mechanism kicked in was 20, while AC aims for 1000[^1]. This commit doesn't go as far as removing these limiters outright, merely disabling them. This is just out of an overabundance of caution. We can probably remove them once kvflowcontrol.enabled has had >1 release worth of baking time. Until then, it's nice to know we have these old safety hatches. We have ample time in the release to assess fallout from this commit, and also use this increased AddSST concurrency to stress the kvflowcontrol machinery. [^1]: The 1000 file limit exists to bound how long it takes to clear L0 completely. Envelope math cribbed from elsewhere: With 2MiB files, 1000 files is ~2GB, which at 40MB/s of compaction throughput (with a compaction slot consistently dedicated to L0) takes < 60s to clear the backlog. So the 'recovery' time is modest in that operators should not need to take manual action Release note: None
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Jul 4, 2023
Fixes cockroachdb#102683. Part of cockroachdb#104154. These were added way back in cockroachdb#36403 and cockroachdb#73904, pre-dating much of IO admission control for leaseholder writes. With cockroachdb#95563, we now have IO admission control for follower writes. Put together, have ample LSM read-amp protection through AC alone. These concurrency limiters are now redundant and oblivious to more sophisticated AC measures. We recently removed the below-raft equivalents of these limiters (cockroachdb#98762), and like mentioned there, these limiters can exacerbate memory pressure. Separately, we're looking to work on speedier restores, and these limiters are starting to get in the way. While here, we also disable the pre-ingest delay mechanism in pebble, which too pre-dates AC, introduced way back in cockroachdb#34258 for RocksDB and in \cockroachdb#41839 for Pebble. IO AC is able to limit the number of L0 files, and this pre-ingest delay with its maximum per-request delay time of 5s can be less than effective. It's worth noting that the L0 file count threshold at which this pre-ingest delay mechanism kicked in was 20, while AC aims for 1000[^1]. This commit doesn't go as far as removing these limiters outright, merely disabling them. This is just out of an overabundance of caution. We can probably remove them once kvflowcontrol.enabled has had >1 release worth of baking time. Until then, it's nice to know we have these old safety hatches. We have ample time in the release to assess fallout from this commit, and also use this increased AddSST concurrency to stress the kvflowcontrol machinery. [^1]: The 1000 file limit exists to bound how long it takes to clear L0 completely. Envelope math cribbed from elsewhere: With 2MiB files, 1000 files is ~2GB, which at 40MB/s of compaction throughput (with a compaction slot consistently dedicated to L0) takes < 60s to clear the backlog. So the 'recovery' time is modest in that operators should not need to take manual action Release note: None
craig bot
pushed a commit
that referenced
this pull request
Jul 4, 2023
104861: kvserver: disable pre-AC above-raft AddSST throttling r=irfansharif a=irfansharif Fixes #102683. Part of #104154. These were added way back in #36403 and #73904, pre-dating much of IO admission control for leaseholder writes. With #95563, we now have IO admission control for follower writes. Put together, have ample LSM read-amp protection through AC alone. These concurrency limiters are now redundant and oblivious to more sophisticated AC measures. We recently removed the below-raft equivalents of these limiters (#98762), and like mentioned there, these limiters can exacerbate memory pressure. Separately, we're looking to work on speedier restores, and these limiters are starting to get in the way. While here, we also disable the pre-ingest delay mechanism in pebble, which too pre-dates AC, introduced way back in #34258 for RocksDB and in \#41839 for Pebble. IO AC is able to limit the number of L0 files, and this pre-ingest delay with its maximum per-request delay time of 5s can be less than effective. It's worth noting that the L0 file count threshold at which this pre-ingest delay mechanism kicked in was 20, while AC aims for 1000[^1]. This commit doesn't go as far as removing these limiters outright, merely disabling them. This is just out of an overabundance of caution. We can probably remove them once kvflowcontrol.enabled has had >1 release worth of baking time. Until then, it's nice to know we have these old safety hatches. We have ample time in the release to assess fallout from this commit, and also use this increased AddSST concurrency to stress the kvflowcontrol machinery. [^1]: The 1000 file limit exists to bound how long it takes to clear L0 completely. Envelope math cribbed from elsewhere: With 2MiB files, 1000 files is ~2GB, which at 40MB/s of compaction throughput (with a compaction slot consistently dedicated to L0) takes < 60s to clear the backlog. So the 'recovery' time is modest in that operators should not need to take manual action. Release note: None Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
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.
This is for 23.2 only
We shouldn't artificially delay SST ingestion below raft because this
exacerbates memory pressure (#81834).
Anecdotally I see in my "typical experiment" (restores under I/O
bandwidth constraints) that
PreIngestDelayis mostly fairly smallcompared to the slowness that comes from write bandwidth overload
itself, so at least in those experiments removing this has little
to no effect.
As we are also working on replication admission control1 and are
looking into improving the memory footprint under unchecked overload2
now's a good time to rip this out as we'll be in a good place to address
any detrimental fallout from doing so.
Touches #81834.
Fixes #57247.
Epic: CRDB-25503
Release note: None
Footnotes
https://github.com/cockroachdb/cockroach/issues/95563 ↩
https://github.com/cockroachdb/cockroach/pull/98576 ↩