intentresolver: use caller's context deadline in intent resolution

[ajwerner]

Having a single timeout value for all batches sent by a request batcher has
proven problematic. The first commit changes the behavior to respect the latest
deadline for a request in a batch. If any request has no deadline, the batch
uses no deadline. The second commit deadline sets a timeout using the previous
value applied to all batches to asynchronous intent resolution calls.

This change isn't exactly equivalent to the prior behavior for async intent
resolution. In particular, the timeout applies to all of the requests rather than
to individual requests. Given the timeout, this seems reasonable. If we fail
to resolve all of the intents we want to within 30s, something else is probably
wrong or we're doing too much work.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 3 of 3 files at r1, 3 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/internal/client/requestbatcher/batcher.go, line 442 at r2 (raw file):

	// sendDeadline is the latest deadline reported by a request's context.
	// It will be zero valued if any request does not	contain a deadline.

weird spacing here

---

pkg/internal/client/requestbatcher/batcher_test.go, line 329 at r1 (raw file):

		// This test attempts to verify that a batch with two requests where one
		// carries a timeout leads to the batch being sent without a timeout.
		// There is a hazard that the goroutines to do the send to not happen

"do not happen"

Can we reduce the frequency of this hazard by bumping up the timeout a little bit?

---

pkg/internal/client/requestbatcher/batcher_test.go, line 347 at r1 (raw file):

			s.respChan <- batchResp{}
		case <-ctx1.Done():
			return

Is this the hazard? If so, let's leave a t.Log so that we have at least a minor chance of catching any regression in the test.

EDIT from r2: yes it looks like it is.

---

pkg/storage/intentresolver/intent_resolver.go, line 59 at r2 (raw file):

	// asyncIntentResolutionTimeout is the timeout when processing a group of
	// intents asynchronously. The timeout prevents intent async intent

"intent async intent"

---

pkg/storage/intentresolver/intent_resolver.go, line 207 at r2 (raw file):

		Stopper:         c.Stopper,
		Sender:          c.DB.NonTransactionalSender(),
		BatchTimeout:    gcTimeout,

Does this build after the first commit? If not, we should fix that. Having a failing build, even in intermediate commits, can mess up git bisects.

[ajwerner]

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

---

pkg/internal/client/requestbatcher/batcher_test.go, line 329 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

"do not happen"

Can we reduce the frequency of this hazard by bumping up the timeout a little bit?

Sure done.

---

pkg/internal/client/requestbatcher/batcher_test.go, line 347 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is this the hazard? If so, let's leave a t.Log so that we have at least a minor chance of catching any regression in the test.

EDIT from r2: yes it looks like it is.

Done.

---

pkg/storage/intentresolver/intent_resolver.go, line 207 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Does this build after the first commit? If not, we should fix that. Having a failing build, even in intermediate commits, can mess up git bisects.

Fixed to remove the setting in the first commit.

[nvanbenschoten]

Reviewed 2 of 3 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)

[ajwerner]

bors r=nvanbenschoten

[craig]

Build succeeded

• GitHub CI (Cockroach)