distsql: remove queueing in the flow scheduler #84888
Merged
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
|
This change is |
yuzefovich
force-pushed
the
remove-flow-scheduler-limit
branch
9 times, most recently
from
81617ca
to
7a6c798
Compare
yuzefovich
added
X-noremind
yuzefovich
force-pushed
the
remove-flow-scheduler-limit
branch
3 times, most recently
from
dc37e43
to
cfc4ef2
Compare
yuzefovich
force-pushed
the
remove-flow-scheduler-limit
branch
from
cfc4ef2
to
aca78e0
Compare
yuzefovich
force-pushed
the
remove-flow-scheduler-limit
branch
2 times, most recently
from
88aee9e
to
438477e
Compare
yuzefovich
removed
do-not-merge
yuzefovich
requested review from
HonoreDB,
michae2 and
cucaroach
and removed request for
a team and
HonoreDB
cucaroach
approved these changes
Oct 10, 2022
There was a problem hiding this comment.
Reviewed 24 of 24 files at r2, 33 of 33 files at r3, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @michae2)
yuzefovich
force-pushed
the
remove-flow-scheduler-limit
branch
from
438477e
to
f08d7c1
Compare
Previously, there was a lot of duplication for two cluster flow tests (one for the single tenant setup and another for the multi tenant setup), and this commit cleans it up. Release note: None
This commit removes the queueing behavior in the flow scheduler (which is now renamed to "remote flow runner" to better reflect its purpose). This behavior was already disabled on 22.2 (with a cluster setting that could enable it back as an escape hatch) since we wanted to be conservative when removing it, but I don't foresee any problems with this, so it should be safe to remove it. We don't remove the "cancel dead flow coordinator" since it might still be useful in a mixed-version cluster, but more importantly the coordinator will be refactored to also cancel the running flows (not just the queued flows as it it used to). (This change will be in a separate commit.) This required removal some of the tests that relied on the queueing behavior, but I don't think we're losing much test coverage. Release note (sql change): `sql.distsql.max_running_flows` cluster setting has been removed. This setting previously controlled the number of remote DistSQL flows that a single node would run at any time. Once that number was exceeded, the incoming flows would get queued until the number was reduced. This was used as a poor man's version of the admission control, but now that we have an actual admission control in place, we don't need that queueing behavior.
yuzefovich
force-pushed
the
remove-flow-scheduler-limit
branch
from
f08d7c1
to
0bbe280
Compare
|
TFTR! bors r+ |
|
Build succeeded: |
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.
flowinfra: de-duplicate cluster flow tests
Previously, there was a lot of duplication for two cluster flow tests
(one for the single tenant setup and another for the multi tenant
setup), and this commit cleans it up.
Release note: None
distsql: remove queueing in the flow scheduler
This commit removes the queueing behavior in the flow scheduler (which
is now renamed to "remote flow runner" to better reflect its purpose).
This behavior was already disabled on 22.2 (with a cluster setting that
could enable it back as an escape hatch) since we wanted to be
conservative when removing it, but I don't foresee any problems with
this, so it should be safe to remove it.
We don't remove the "cancel dead flow coordinator" since it might still
be useful in a mixed-version cluster, but more importantly the
coordinator will be refactored to also cancel the running flows (not
just the queued flows as it it used to). (This change will be in
a separate commit.)
This required removal some of the tests that relied on the queueing
behavior, but I don't think we're losing much test coverage.
Fixes: #34229.
Release note (sql change):
sql.distsql.max_running_flowsclustersetting has been removed. This setting previously controlled the number
of remote DistSQL flows that a single node would run at any time. Once
that number was exceeded, the incoming flows would get queued until the
number was reduced. This was used as a poor man's version of the
admission control, but now that we have an actual admission control in
place, we don't need that queueing behavior.