Skip to content

kv: introduce request RoutingPolicy configuration#68191

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/routingPolicy
Aug 5, 2021
Merged

kv: introduce request RoutingPolicy configuration#68191
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/routingPolicy

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Jul 28, 2021

Half of #67551.
Touches #67562.

This commit introduces a new RoutingPolicy configuration that lives on a BatchRequest header. A request's routing policy specifies how the request should be routed to the replicas of its target range(s) by the DistSender. There are initially two routing policies:

enum RoutingPolicy {
  // LEASEHOLDER means that the DistSender should route the request to the
  // leaseholder replica(s) of its target range(s).
  LEASEHOLDER = 0;
  // NEAREST means that the DistSender should route the request to the
  // nearest replica(s) of its target range(s).
  NEAREST = 1;
}

The default policy is LEASEHOLDER.

Routing policies allow us to stop overloading the use of the ReadConsistency enum to dictate both how the client should route a request to a server and which kinds of requests should be eligible to be served by a given replica. Routing policies are a client-side only concept. They do not dictate which replicas in a range are eligible to serve the request, only which replicas are considered as targets by the DistSender, and in which order. A request that is routed to an ineligible replica (a function of request type, timestamp, and read consistency) will be rejected by that replica and the DistSender will target another replica in the range.

As discussed in #67725 (review), we will likely need to introduce a third routing policy called SINGLE_REPLICA to address #67554. This policy would be accompanied by a ReplicaDescriptor and would specify that a given request must be sent to that replica and DistSender should throw an error if the replica is not part of its cached range descriptor. This is important to ensure that a QueryResolvedTimestampRequest and its follow-up ScanRequest are both sent to the same replica.

@nvb nvb requested review from a team, irfansharif and tbg July 28, 2021 18:57
@nvb nvb requested a review from a team as a code owner July 28, 2021 18:57
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@tbg
Copy link
Copy Markdown
Member

tbg commented Jul 29, 2021

Ran out of time for this review while wrapping up loose ends before PTO, sorry Nathan! I won't be reviewing this until the week of Aug 16th at which point you'll surely already have it merged.

Copy link
Copy Markdown
Contributor

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 11 of 11 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)


pkg/kv/kvclient/kvcoord/dist_sender.go, line 1821 at r1 (raw file):

		// Order by latency.
		log.VEvent(ctx, 2, "routing to nearest replica; leaseholder not required")
		replicas.OptimizeReplicaOrder(ds.getNodeDescriptor(), ds.latencyFunc)

Missing a ds.dontRorderReplicas check here.


pkg/kv/kvserver/replica.go, line 1293 at r1 (raw file):

	}

	// Is the lease valid?

Comment here needs updating.

Touches cockroachdb#67562.

This commit introduces a new RoutingPolicy configuration that lives on a
BatchRequest header. A request's routing policy specifies how the
request should be routed to the replicas of its target range(s) by the
DistSender. There are initially two routing policies:
```
enum RoutingPolicy {
  // LEASEHOLDER means that the DistSender should route the request to the
  // leaseholder replica(s) of its target range(s).
  LEASEHOLDER = 0;
  // NEAREST means that the DistSender should route the request to the
  // nearest replica(s) of its target range(s).
  NEAREST = 1;
}
```

The default policy is `LEASEHOLDER`.

Routing policies allow us to stop overloading the use of the
ReadConsistency enum to dictate both how the client should route a
request to a server and which kinds of requests should be eligible to be
served by a given replica. Routing policies are a client-side only
concept. They do not dictate which replicas in a range are eligible to
serve the request, only which replicas are considered as targets by the
DistSender, and in which order. A request that is routed to an
ineligible replica (a function of request type, timestamp, and read
consistency) will be rejected by that replica and the DistSender will
target another replica in the range.

As discussed in cockroachdb#67725 (review),
we will likely need to introduce a third routing policy that called
`SINGLE_REPLICA` to address cockroachdb#67554. This policy would be accompanied by
a ReplicaDescriptor and would specify that a given request must be sent
to that replica and DistSender should throw an error if the replica is
not part of its cached range descriptor. This is important to ensure
that a QueryResolvedTimestampRequest and its follow-up ScanRequest are
both sent to the same replica.
@nvb nvb force-pushed the nvanbenschoten/routingPolicy branch from 0c56d6b to 79a956f Compare August 5, 2021 15:55
Copy link
Copy Markdown
Contributor Author

@nvb nvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif)


pkg/kv/kvclient/kvcoord/dist_sender.go, line 1821 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Missing a ds.dontRorderReplicas check here.

Done.

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 5, 2021

Build succeeded:

@craig craig bot merged commit 380660d into cockroachdb:master Aug 5, 2021
@nvb nvb deleted the nvanbenschoten/routingPolicy branch August 5, 2021 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants