kv: permit ExportRequest evaluation on followers

[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

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Nice!

@dt do you think there are any other backup-level tests that should be updated or added now that this is supported? Are there any tests that you would expect this change to have an impact on? For instance, should we be able to see the impact of this on intra-cluster network transfer in a long-running backup roachtest (where leases may move after the backup is planned)?

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

---

pkg/kv/kvserver/replica_follower_read.go line 46 at r1 (raw file):

	//    read, then the batch might miss past writes served at higher timestamps
	//    on the leaseholder.
	// 2. Each request in the batch needs to have clearly defined semantics when

small nit: If we're going to capitalize this sentence, let's do it for all of these conditions.

---

pkg/kv/kvserver/replica_follower_read.go line 48 at r1 (raw file):

	// 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).

"which are non-transactional but which define a start and end timestamp"

---

pkg/roachpb/batch.go line 195 at r1 (raw file):

}

// IsAllTransactionalOrExportRequests returns true iff every request in the

Is this condition applicable more broadly than to just BatchCanBeEvaluatedOnFollower? If not, consider inlining it into BatchCanBeEvaluatedOnFollower. It might look like:

...
if tsFromServerClock {
	return false
}
for _, ru := range ba.Requests {
	r := ru.GetInner()
	switch {
	case roachpb.IsTransactional(r):
		// TODO: explain
		if !roachpb.IsReadOnly(r) || roachpb.IsLocking(r) {
			return false
		}
	case r.Method() == roachpb.Export:
		// TODO: explain
	default:
		return false
	}
}
return true

[arulajmani]

Sorry for taking so long to get to the review comments; updated, PTAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/kv/kvserver/replica_follower_read.go line 46 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

small nit: If we're going to capitalize this sentence, let's do it for all of these conditions.

No longer applies because I moved the explanation in-line. Let me know if you prefer otherwise.

---

pkg/roachpb/batch.go line 195 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is this condition applicable more broadly than to just BatchCanBeEvaluatedOnFollower? If not, consider inlining it into BatchCanBeEvaluatedOnFollower. It might look like:

...
if tsFromServerClock {
	return false
}
for _, ru := range ba.Requests {
	r := ru.GetInner()
	switch {
	case roachpb.IsTransactional(r):
		// TODO: explain
		if !roachpb.IsReadOnly(r) || roachpb.IsLocking(r) {
			return false
		}
	case r.Method() == roachpb.Export:
		// TODO: explain
	default:
		return false
	}
}
return true

Good call, this is much cleaner.

[nvanbenschoten]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

---

pkg/kv/kvserver/replica_follower_read.go line 68 at r2 (raw file):

			// leaseholder.
			// Explanation of conditions required for a batch to be considered for
			// follower evaluation are defined inline.

Explanation of conditions required for a batch to be considered for follower evaluation are defined inline.

What is this sentence intending to convey? Didn't you just explain these conditions above?

---

pkg/kv/kvserver/replica_follower_read.go line 74 at r2 (raw file):

		case r.Method() == roachpb.Export:
		// Export requests also have clear semantics when served under the closed
		// timestamp as well, even though they aren't non-transactional, as they

"are non-transactional" or "aren't transactional"

[arulajmani]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/kv/kvserver/replica_follower_read.go line 68 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Explanation of conditions required for a batch to be considered for follower evaluation are defined inline.

What is this sentence intending to convey? Didn't you just explain these conditions above?

Lol I think I accidentally pressed p here and I had that on my clipboard from before. Fixed.

---

pkg/kv/kvserver/replica_follower_read.go line 74 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

"are non-transactional" or "aren't transactional"

Done.

[nvanbenschoten]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani)

[arulajmani]

TFTR!

bors r=nvanbenschoten

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)