-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
kvserver: remove above-raft throttling of AddSST #101530
Conversation
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. |
@irfansharif could you give me an explanation on why we're now fine removing this? Because we have sufficient integration with admission control? Or because we just don't think this will do more harm than good? |
7685818
to
f147be1
Compare
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
Epic: none Release note: None
f147be1
to
d25a57f
Compare
Friendly ping. |
I miscommunicated the sequencing, I don't think we should get rid of these for at least a few weeks. Not until #98308 has landed and has had chance to bake on master as enabled. This is just out of an abundance of caution and not inviting more CI fallout than necessary. What I can do instead is assign myself this PR, mark it as draft, and take it over the finish line when ready. |
Yes to both, after the kvflowcontrol machinery is used by default, we should have sufficient protection using just AC machinery and I don't think these limiters would really be meaningful. |
👍🏽 Would you mind re-opening under your name? That way it doesn't dangle around in my workflows. Thanks! Filed #102683 to track and assigned to you. |
We now rely on admission control to protect the nodes.1
Epic: none
Release note (ops change): The following cluster settings were retired and no longer have any effect:
Footnotes
https://github.com/cockroachdb/cockroach/pull/98762#discussion_r1164701947 ↩