Skip to content

Commit

Permalink
Merge #91405 #93124
Browse files Browse the repository at this point in the history 
91405: kv: permit ExportRequest evaluation on followers r=nvanbenschoten a=arulajmani

Now that we've fixed #67016, and follower reads correctly synchronize with concurrent splits, it's safe for us to serve ExportRequests from followers. This patch permits that.

Closes #88804

Release note: None

93124: roachtest: check logger file before dereference r=smg260 a=smg260

Another nil dereferece fix like #92845.

[Error in TC](https://teamcity.cockroachdb.com/viewLog.html?buildId=7824979&buildTypeId=Cockroach_Nightlies_RoachtestNightlyGceBazel)

Release note: none
Epic: none

Co-authored-by: Arul Ajmani <[email protected]>
Co-authored-by: Miral Gadani <[email protected]>
  • Loading branch information
@arulajmani
3 people committed Dec 7, 2022
3 parents f860413 + df6f558 + 2a9ac2c commit fd6b899
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 21 deletions.
27 changes: 22 additions & 5 deletions pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go
View file
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: 6 additions & 2 deletions pkg/cmd/roachtest/test_impl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,9 +357,13 @@ func (t *testImpl) addFailure(format string, args ...interface{}) {
// We don't actually log through this logger since it adds an unrelated
// file:line caller (namely ours). The error already has stack traces
// so it's better to write only it to the file to avoid confusion.
path := cl.File.Name()
if cl.File != nil {
path := cl.File.Name()
if len(path) > 0 {
_ = os.WriteFile(path, []byte(fmt.Sprintf("%+v", reportFailure.squashedErr)), 0644)
}
}
cl.Close() // we just wanted the filename
_ = os.WriteFile(path, []byte(fmt.Sprintf("%+v", reportFailure.squashedErr)), 0644)
}
}

Expand Down
49 changes: 35 additions & 14 deletions pkg/kv/kvserver/replica_follower_read.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,24 +37,45 @@ var FollowerReadsEnabled = settings.RegisterBoolSetting(
// requests that can be evaluated on a follower replica, given a sufficiently
// advanced closed timestamp.
func BatchCanBeEvaluatedOnFollower(ba *roachpb.BatchRequest) bool {
// Explanation of conditions:
// 1. the batch cannot have or intend to receive a timestamp set from a
// server-side clock. If a follower with a lagging clock sets its timestamp
// 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.
// 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
// held on the leaseholder.
// Various restrictions apply to a batch for it to be successfully considered
// for evaluation on a follower replica, which are described inline.
//
// The batch cannot have or intend to receive a timestamp set from a
// server-side clock. If follower with a lagging clock sets its timestamp
// 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.
tsFromServerClock := ba.Txn == nil && (ba.Timestamp.IsEmpty() || ba.TimestampFromServerClock != nil)
if tsFromServerClock {
return false
}
return ba.IsAllTransactional() && ba.IsReadOnly() && !ba.IsLocking()
if len(ba.Requests) == 0 {
// No requests to evaluate.
return false
}
// Each request in the batch needs to have clearly defined semantics when
// served under the closed timestamp.
for _, ru := range ba.Requests {
r := ru.GetInner()
switch {
case roachpb.IsTransactional(r):
// Transactional requests have clear semantics when served under the
// closed timestamp. The request must be read-only, as follower replicas
// cannot propose writes to Raft. The request also needs to be
// non-locking, because unreplicated locks are only held on the
// leaseholder.
if !roachpb.IsReadOnly(r) || roachpb.IsLocking(r) {
return false
}
case r.Method() == roachpb.Export:
// Export requests also have clear semantics when served under the closed
// timestamp as well, even though they are non-transactional, as they
// define the start and end timestamp to export data over.
default:
return false
}
}
return true
}

// canServeFollowerReadRLocked tests, when a range lease could not be acquired,
Expand Down

0 comments on commit fd6b899

Please sign in to comment.