changefeedccl: repopulate request body for webhook sink retries #119326
Conversation
rharding6373
requested review from
jayshrivastava
and removed request for
a team
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jayshrivastava)
pkg/ccl/changefeedccl/sink_webhook_test.go line 735 at r1 (raw file):
}{ key: changefeedbase.OptWebhookSinkConfig, value: `{"Retry":{"Backoff": "1s", "Max": "2"}}`})
Without the code change, this test doesn't reliably fail unless the Backoff is ~60s (which requires a change to sink options to have a retry max backoff of >60s). In the debugger, I see the error in the http library but it seems like it takes time to be consumed and propagated and I don't fully understand why yet. I don't think we want to adjust the retry max backoff or expose it as an option just to get this test working well. Since this change has been tested manually and with an adjusted backoff, I'm confident that it fixes the problem, but I'm sad that the unit test isn't consistent. Thoughts?
| key string | ||
| value string | ||
| }{ | ||
| key: changefeedbase.OptWebhookSinkConfig, |
There was a problem hiding this comment.
Does the test take longer to run with these options? If so, using 0 backoff would be better.
rharding6373
force-pushed
the
20240213_webhook_retry_118485
branch
from
8507bcf
to
1a2d035
Compare
rharding6373
added
backport-23.1.x
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/ccl/changefeedccl/sink_webhook_test.go line 734 at r1 (raw file):
Previously, jayshrivastava (Jayant) wrote…
Does the test take longer to run with these options? If so, using 0 backoff would be better.
I don't think it will repro with 0 backoff, but I did reduce it so that the test can run in ~1s. I also added a comment to explain the repro issues we discussed.
The webhook sink reuses an http request for retries. However, the http library consumes the request body, so retries may result in an error due to a non-zero content length but zero length body. This change re-initializes the http request body on every request so that it will have the right contents on every retry. Epic: none Fixes: cockroachdb#118485 Release note (bug fix): Fixes a bug in the webhook sink where the http request body may not be initialized on retries, resulting in the error "http: ContentLength=... with Body length 0".
rharding6373
force-pushed
the
20240213_webhook_retry_118485
branch
from
1a2d035
to
7ade4de
Compare
|
TFTR! bors r=jayshrivastava |
|
Build succeeded: |
The webhook sink reuses an http request for retries. However, the http library consumes the request body, so retries may result in an error due to a non-zero content length but zero length body. This change re-initializes the http request body on every request so that it will have the right contents on every retry.
Epic: none
Fixes: #118485
Release note (bug fix): Fixes a bug in the webhook sink where the http request body may not be initialized on retries, resulting in the error "http: ContentLength=... with Body length 0".