requestbatcher,intentresolver: add timeout in requestbatcher and adopt#36845
Conversation
|
This change is |
|
Before #34803 we used to also respect client provided timeouts via the context passed to the intent resolver if it was shorter than the package default. We could add logic to do that still but I wasn't sure what the right policy would be. It seems like if the code were to give any consideration to client provided timeouts the two options would be to respect the earliest timeout of any request sent in the batch or the latest. The earliest worries me as it's probably not a good idea to cancel a whole batch for a single request due perhaps to a timeout from processing the gc queue. Taking the latest client provided timeout seems fine but I don't have a compelling argument to justify the complexity. |
501ca42 to
8de9061
Compare
|
Added another commit which changes the behavior of contextutil.RunWithTimeout so that it returns a TimeoutError if the timeout is exceeded even if the returned error does not have context.DeadlineExceeded as its Cause. See the commit message for more details. |
nvb
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 3 of 3 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/internal/client/requestbatcher/batcher.go, line 272 at r2 (raw file):
defer b.sendDone(ba) var resp *roachpb.BatchResponse var pErr *roachpb.Error
Are we getting anything out of capturing pErr? If not then I'd tighten its scope both for clarity and to ensure that it doesn't escape.
pkg/util/contextutil/context_test.go, line 83 at r1 (raw file):
notContextDeadlineExceeded := errors.New(context.DeadlineExceeded.Error()) err := RunWithTimeout(ctx, "foo", 1, func(ctx context.Context) error { time.Sleep(10 * time.Millisecond)
This looks flaky. Can't we wait on the context cancelation like: <-ctx.Done()?
I agree that it's not worth the complexity. What you have here looks fine. |
Before this commit, if the context were to have expired but the returned errors did not have a Cause of context.DeadlineExceeded, RunWithTimeout would not return a TimeoutError. It is perhaps surprising that the returned TimeoutError does not have context.DeadlineExceeded as its Cause but it seemed preferable to retain the potentially more meaningful error as the Cause.
This PR adds a configuration option for a timeout for batch request sends in the request batcher and adopts this option in the intent resolver. Prior to the introduction of batching in cockroachdb#34803 intent resolution was performed with a maximum timeout of 30s. This PR uses that same constant and additionally applies it to batches of requests to gc transaction records. Informs cockroachdb#36806. Release note: None
8de9061 to
be3414d
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/internal/client/requestbatcher/batcher.go, line 272 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Are we getting anything out of capturing
pErr? If not then I'd tighten its scope both for clarity and to ensure that it doesn't escape.
Done.
pkg/util/contextutil/context_test.go, line 83 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
This looks flaky. Can't we wait on the context cancelation like:
<-ctx.Done()?
Done.
|
bors r+ |
36845: requestbatcher,intentresolver: add timeout in requestbatcher and adopt r=ajwerner a=ajwerner This PR adds a configuration option for a timeout for batch request sends in the request batcher and adopts this option in the intent resolver. Prior to the introduction of batching in #34803 intent resolution was performed with a maximum timeout of 30s. This PR uses that same constant and additionally applies it to batches of requests to gc transaction records. Informs #36806. Release note: None Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>
Build succeeded |
This PR adds a configuration option for a timeout for batch request sends in
the request batcher and adopts this option in the intent resolver. Prior to
the introduction of batching in #34803 intent resolution was performed with a
maximum timeout of 30s. This PR uses that same constant and additionally
applies it to batches of requests to gc transaction records.
Informs #36806.
Release note: None