Skip to content

Commit

Permalink
kv: permit ExportRequest evaluation on followers
Browse files Browse the repository at this point in the history
Now that we've fixed cockroachdb#67016, and follower reads correctly synchronize
with concurrent splits, it's safe for us to serve ExportRequests from
followers. This patch permits that.

Closes cockroachdb#88804

Release note: None
  • Loading branch information
arulajmani committed Nov 7, 2022
1 parent e01f162 commit 0c460c5
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 9 deletions.
27 changes: 22 additions & 5 deletions pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,12 @@ func TestCanSendToFollower(t *testing.T) {
txn.GlobalUncertaintyLimit = ts
return txn
}
batch := func(txn *roachpb.Transaction, req roachpb.Request) *roachpb.BatchRequest {
batch := func(txn *roachpb.Transaction, reqs ...roachpb.Request) *roachpb.BatchRequest {
ba := &roachpb.BatchRequest{}
ba.Txn = txn
ba.Add(req)
for _, req := range reqs {
ba.Add(req)
}
return ba
}
withBatchTimestamp := func(ba *roachpb.BatchRequest, ts hlc.Timestamp) *roachpb.BatchRequest {
Expand Down Expand Up @@ -146,7 +148,17 @@ func TestCanSendToFollower(t *testing.T) {
{
name: "stale non-txn export batch",
ba: withBatchTimestamp(batch(nil, &roachpb.ExportRequest{}), stale),
exp: false,
exp: true,
},
{
name: "stale non-txn multiple exports batch",
ba: withBatchTimestamp(batch(nil, &roachpb.ExportRequest{}, &roachpb.ExportRequest{}), stale),
exp: true,
},
{
name: "stale non-txn mixed export-scan batch",
ba: withBatchTimestamp(batch(nil, &roachpb.ExportRequest{}, &roachpb.ScanRequest{}), stale),
exp: true,
},
{
name: "current-time non-txn batch",
Expand All @@ -158,6 +170,11 @@ func TestCanSendToFollower(t *testing.T) {
ba: withBatchTimestamp(batch(nil, &roachpb.ExportRequest{}), current),
exp: false,
},
{
name: "current-time non-txn multiple exports batch",
ba: withBatchTimestamp(batch(nil, &roachpb.ExportRequest{}, &roachpb.ExportRequest{}), current),
exp: false,
},
{
name: "future non-txn batch",
ba: withBatchTimestamp(batch(nil, &roachpb.GetRequest{}), future),
Expand Down Expand Up @@ -275,7 +292,7 @@ func TestCanSendToFollower(t *testing.T) {
name: "stale non-txn export batch, global reads policy",
ba: withBatchTimestamp(batch(nil, &roachpb.ExportRequest{}), stale),
ctPolicy: roachpb.LEAD_FOR_GLOBAL_READS,
exp: false,
exp: true,
},
{
name: "current-time non-txn batch, global reads policy",
Expand All @@ -287,7 +304,7 @@ func TestCanSendToFollower(t *testing.T) {
name: "current-time non-txn export batch, global reads policy",
ba: withBatchTimestamp(batch(nil, &roachpb.ExportRequest{}), current),
ctPolicy: roachpb.LEAD_FOR_GLOBAL_READS,
exp: false,
exp: true,
},
{
name: "future non-txn batch, global reads policy",
Expand Down
8 changes: 4 additions & 4 deletions pkg/kv/kvserver/replica_follower_read.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ func BatchCanBeEvaluatedOnFollower(ba *roachpb.BatchRequest) bool {
// and this then allows the follower to evaluate the batch as a follower
// read, then the batch might miss past writes served at higher timestamps
// on the leaseholder.
// 2. each request in the batch needs to be "transactional", because those are
// the only ones that have clearly defined semantics when served under the
// closed timestamp.
// 2. Each request in the batch needs to have clearly defined semantics when
// served under the closed timestamp. This includes all "transactional"
// requests and ExportRequests (which are non-transactional).
// 3. the batch needs to be read-only, because a follower replica cannot
// propose writes to Raft.
// 4. the batch needs to be non-locking, because unreplicated locks are only
Expand All @@ -54,7 +54,7 @@ func BatchCanBeEvaluatedOnFollower(ba *roachpb.BatchRequest) bool {
if tsFromServerClock {
return false
}
return ba.IsAllTransactional() && ba.IsReadOnly() && !ba.IsLocking()
return ba.IsAllTransactionalOrExportRequests() && ba.IsReadOnly() && !ba.IsLocking()
}

// canServeFollowerReadRLocked tests, when a range lease could not be acquired,
Expand Down
14 changes: 14 additions & 0 deletions pkg/roachpb/batch.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,20 @@ func (ba *BatchRequest) IsAllTransactional() bool {
return ba.hasFlagForAll(isTxn)
}

// IsAllTransactionalOrExportRequests returns true iff every request in the
// BatchRequest can either be part of a transaction or is an ExportRequest.
func (ba *BatchRequest) IsAllTransactionalOrExportRequests() bool {
if len(ba.Requests) == 0 {
return false
}
for _, union := range ba.Requests {
if (union.GetInner().flags()&isTxn) == 0 && union.GetInner().Method() != Export {
return false
}
}
return true
}

// IsLocking returns true iff the BatchRequest intends to acquire locks.
func (ba *BatchRequest) IsLocking() bool {
return ba.hasFlag(isLocking)
Expand Down

0 comments on commit 0c460c5

Please sign in to comment.