Skip to content
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

kvclient: don't send follower reads to unhealthy nodes #112351

Closed
andrewbaptist opened this issue Oct 13, 2023 · 2 comments · Fixed by #114205
Closed

kvclient: don't send follower reads to unhealthy nodes #112351

andrewbaptist opened this issue Oct 13, 2023 · 2 comments · Fixed by #114205
Assignees
Labels
A-kv-client Relating to the KV client and the KV interface. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs T-kv KV Team

Comments

@andrewbaptist
Copy link
Collaborator

andrewbaptist commented Oct 13, 2023

Describe the problem

A draining node is likely to shut down soon. By moving the leaseholders off we stop all non-follower reads to that node, but we don't prevent follower reads from using it. SInce we already know that it is draining, we should either exclude or prioritize last the draining or suspect nodes. This could be enhanced to also exclude nodes with IO overload or generally take the health of the node into account when deciding which node to send a follower read to.

To Reproduce

  1. Create a CRDB cluster with 6 nodes.
  2. Run a workload of only follower reads at a low rate.
  3. Drain and then stop one of the nodes.
  4. Notice the P99.99 ~3s latency at the time the node is stopped due to read attempts waiting for an RPC timeout.

Expected behavior

From a client perspective, draining a node should allow clean shutdown without a performance impact. Follower reads break this model because while all the leases are moved, so normal reads and writes do not require this node, follower reads don't take this into account.

Additional context
This was noticed in a customer investigation where they had a 500ms client timeout on read requests and saw an elevated number of failures when they were cycling nodes.

Jira issue: CRDB-32365

@andrewbaptist andrewbaptist added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Oct 13, 2023
@lyang24
Copy link
Contributor

lyang24 commented Oct 19, 2023

Excluding the draining node might be easier approach compare to tweak with replica sorting.

If we are adding to node health or io overload to the sorting equation. Should we make modifications to RoutingPolicy enum to reflect new semantic as well? I guess the sorting logic maybe be in OptimizeReplicaOrder function but I am not experienced enough to look for where to collect node health and IO overload info.

This is an interesting issue - I'm excited to read over the future pr that fixes this issue.

@andrewbaptist andrewbaptist self-assigned this Oct 20, 2023
@andrewbaptist andrewbaptist added O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs A-kv-client Relating to the KV client and the KV interface. T-kv KV Team labels Oct 26, 2023
andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Oct 26, 2023
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 added a commit to andrewbaptist/cockroach that referenced this issue Nov 1, 2023
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 added a commit to andrewbaptist/cockroach that referenced this issue Nov 1, 2023
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.
@amaddahian
Copy link

This issue was discussed on Roblox monthly call and has been flagged as a blocker for which we need to provide an ETA and/or target release in 23.x.

andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Nov 6, 2023
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 added a commit to andrewbaptist/cockroach that referenced this issue Nov 9, 2023
This PR replaces ReplicaSlice with ReplicaSet outside of the
DistSender. ReplicaSlice is an internal implementation which
contains information only required for sorting the replicas.
Outside of DistSender the additional sorting information is
unnecessary and unused.

Epic: none
Informs: cockroachdb#112351

Release note: None
craig bot pushed a commit that referenced this issue Nov 13, 2023
113942: kvclient: optimize and clean up sorting computation r=nvanbenschoten a=andrewbaptist

Previously the locality distance and the latency function were computed multiple times for each node in the sort.Slice method. This change computes the values once when the ReplicaSlice is created and uses simple comparisions within the sorting loop.

Epic: none
Informs: #112351

Release note: None

114240: rangefeed: fix scheduler catchup iterator race r=erikgrinaker a=erikgrinaker

It was possible for the scheduled processor to hand ownership of the catchup iterator over to the registration, but claim that it didn't by returning `false` from `Register()`.

This can happen if the registration request is queued concurrently with a processor shutdown, where the registration will execute the catchup scan and close the iterator, but the caller will think it wasn't registered and double-close the iterator.

This patch fixes the race, and also documents the necessary invariant along with a runtime assertion.

Resolves #114192.
Epic: none
Release note: None

114309: logictest: add test for mixed-version configs r=RaduBerinde a=RaduBerinde

This commit adds a test that verifies that for each supported previous release we have a logictest config that bootstraps the cluster at that version.

Informs: #112629
Release note: None

Co-authored-by: Andrew Baptist <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Nov 14, 2023
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 added a commit to andrewbaptist/cockroach that referenced this issue Nov 14, 2023
This PR replaces ReplicaSlice with ReplicaSet outside of the
DistSender. ReplicaSlice is an internal implementation which
contains information only required for sorting the replicas.
Outside of DistSender the additional sorting information is
unnecessary and unused.

Epic: none
Informs: cockroachdb#112351

Release note: None
andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Dec 18, 2023
This PR replaces ReplicaSlice with ReplicaSet outside of the
DistSender. ReplicaSlice is an internal implementation which
contains information only required for sorting the replicas.
Outside of DistSender the additional sorting information is
unnecessary and unused.

Epic: none
Informs: cockroachdb#112351

Release note: None
andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Jan 19, 2024
This PR replaces ReplicaSlice with ReplicaSet outside of the
DistSender. ReplicaSlice is an internal implementation which
contains information only required for sorting the replicas.
Outside of DistSender the additional sorting information is
unnecessary and unused.

Epic: none
Informs: cockroachdb#112351

Release note: None
andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Jan 25, 2024
This PR replaces ReplicaSlice with ReplicaSet outside of the
DistSender. ReplicaSlice is an internal implementation which
contains information only required for sorting the replicas.
Outside of DistSender the additional sorting information is
unnecessary and unused.

Epic: none
Informs: cockroachdb#112351

Release note: None
andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Jan 25, 2024
This PR replaces ReplicaSlice with ReplicaSet outside of the
DistSender. ReplicaSlice is an internal implementation which
contains information only required for sorting the replicas.
Outside of DistSender the additional sorting information is
unnecessary and unused.

Epic: none
Informs: cockroachdb#112351

Release note: None
andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Jan 26, 2024
This PR replaces ReplicaSlice with ReplicaSet outside of the
DistSender. ReplicaSlice is an internal implementation which
contains information only required for sorting the replicas.
Outside of DistSender the additional sorting information is
unnecessary and unused.

Epic: none
Informs: cockroachdb#112351

Release note: None
andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Jan 26, 2024
This PR replaces ReplicaSlice with ReplicaSet outside of the
DistSender. ReplicaSlice is an internal implementation which
contains information only required for sorting the replicas.
Outside of DistSender the additional sorting information is
unnecessary and unused.

Epic: none
Informs: cockroachdb#112351

Release note: None
andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Jan 29, 2024
This PR replaces ReplicaSlice with ReplicaSet outside of the
DistSender. ReplicaSlice is an internal implementation which
contains information only required for sorting the replicas.
Outside of DistSender the additional sorting information is
unnecessary and unused.

Epic: none
Informs: cockroachdb#112351

Release note: None
andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Jan 29, 2024
This PR replaces ReplicaSlice with ReplicaSet outside of the
DistSender. ReplicaSlice is an internal implementation which
contains information only required for sorting the replicas.
Outside of DistSender the additional sorting information is
unnecessary and unused.

Epic: none
Informs: cockroachdb#112351

Release note: None
andrewbaptist added a commit to andrewbaptist/cockroach that referenced this issue Mar 6, 2024
This PR replaces ReplicaSlice with ReplicaSet outside of the
DistSender. ReplicaSlice is an internal implementation which
contains information only required for sorting the replicas.
Outside of DistSender the additional sorting information is
unnecessary and unused.

Epic: none
Informs: cockroachdb#112351

Release note: None
@github-project-automation github-project-automation bot moved this to Closed in KV Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-client Relating to the KV client and the KV interface. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs T-kv KV Team
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants