kvclient: stop reads on followers #114205
Conversation
|
This change is |
andrewbaptist
force-pushed
the
2023-11-09-follower-reads-simple
branch
from
93f9755 to
e3bdff2
Compare
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist and @mgartner)
pkg/kv/kvclient/kvcoord/dist_sender.go line 278 at r1 (raw file):
"kv.dist_sender.follower_reads_unhealthy.enabled", "send follower reads to unhealthy nodes", true,
We want to start with this set to false, right? Until we've mitigated the performance impact. We can then set it to true in the tests which want to exercise it.
I guess the downside of that is that we get less passive testing of the logic. We don't want certain customers running with logic that we only test in rare cases.
Ok, I guess it seems ok to keep this set to true on master to ensure it gets full test coverage, and then set it to false in the backport.
EDIT: I was actually misinterpreting this setting. I thought "true" meant "opt into the new behavior". Landing this on master defaulted to off (true) seems like the right approach. I think we may still want to get it turned on by default on master before backporting the off-by-default version to the release branch, so we know that it's at least getting full test coverage on some branch.
Does that seem like a reasonable plan?
pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go line 1184 at r1 (raw file):
Settings: settings, Locality: localities[i], DefaultTestTenant: base.TestIsSpecificToStorageLayerAndNeedsASystemTenant,
When we backport, we'll just not include this, right? Are there any downsides to that, other than the test taking a little longer?
Stop follower reads on draining, decommissioning or unhealthy nodes. Epic: none Fixes: cockroachdb#112351 Release note (performance improvement): This change prevents failed requests from being issued on followers that are draining, decommissioning or unhealthy which prevents latency spikes if those nodes later go offline.
andrewbaptist
force-pushed
the
2023-11-09-follower-reads-simple
branch
from
e3bdff2 to
3111a55
Compare
There was a problem hiding this comment.
TFTR - Running one more time with the removal of the TestArgs so it can be an "exact" backport. It should finish shortly.
Reviewable status:
pkg/kv/kvclient/kvcoord/dist_sender.go line 278 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
We want to start with this set to false, right? Until we've mitigated the performance impact. We can then set it to true in the tests which want to exercise it.
I guess the downside of that is that we get less passive testing of the logic. We don't want certain customers running with logic that we only test in rare cases.
Ok, I guess it seems ok to keep this set to true on master to ensure it gets full test coverage, and then set it to false in the backport.
EDIT: I was actually misinterpreting this setting. I thought "true" meant "opt into the new behavior". Landing this on master defaulted to off (true) seems like the right approach. I think we may still want to get it turned on by default on master before backporting the off-by-default version to the release branch, so we know that it's at least getting full test coverage on some branch.
Does that seem like a reasonable plan?
Yes - this is exactly what I was thinking. Leaving this as "disabled" (reading from unhealthy) no both 23.2 and master and then enabling once the other PRs land.
pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go line 1184 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
When we backport, we'll just not include this, right? Are there any downsides to that, other than the test taking a little longer?
I'm removing these settings so I don't need to wait for the other PR. I'll reenable these later on master in a different PR. It does just take a little longer, although I've seen tests flake on stress+race without these.
We have a somewhat annoying situation that tests only run StressRace after a change has been made to them, and I believe a number of tests (maybe a majority) will fail the first time any change is made. We probably should figure out a better way to handle this.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
bors r=nvanbenschoten |
|
Build succeeded: |
andrewbaptist
added
the
backport-23.2.x
|
blathers backport 23.2 |
Stop follower reads on draining, decommissioning or unhealthy nodes.
Epic: none
Fixes: #112351
Release note (performance improvement): This change prevents failed requests from being issued on followers that are draining, decommissioning or unhealthy which prevents latency spikes if those nodes later go offline.