changefeedccl: retry webhook sink requests upon HTTP error#67686
changefeedccl: retry webhook sink requests upon HTTP error#67686craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
|
||
| func defaultRetryConfig() retryConfig { | ||
| return retryConfig{ | ||
| backoff: 500 * time.Millisecond, |
There was a problem hiding this comment.
open to discussing this default config further (similar to kafka except backoff 250 -> 500)
f3f2611 to
8e768d5
Compare
8e768d5 to
292d84e
Compare
292d84e to
0f051a3
Compare
miretskiy
left a comment
There was a problem hiding this comment.
Reviewed 1 of 4 files at r5, 2 of 6 files at r6.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @spiffyyeng)
pkg/ccl/changefeedccl/sink_webhook.go, line 213 at r6 (raw file):
} func defaultRetryConfig() retry.Options {
add TODO to make this configurable ?
pkg/ccl/changefeedccl/sink_webhook.go, line 220 at r6 (raw file):
} // max backoff should be initial * 2 ^ maxRetries opts.MaxBackoff = opts.InitialBackoff * time.Duration(int(math.Pow(2.0, float64(opts.MaxRetries))))
do you need to set this? Do we need such high precision?
pkg/ccl/changefeedccl/sink_webhook.go, line 266 at r6 (raw file):
func (s *webhookSink) sendMessageWithRetries(ctx context.Context, reqBody []byte) error { requestFunc := func() error { err := s.sendMessage(ctx, reqBody)
just return s.sendMessge(ctx, reqBody)?
pkg/ccl/changefeedccl/sink_webhook_test.go, line 386 at r6 (raw file):
Quoted 4 lines of code…
err = sinkSrc.Close() if err != nil { t.Fatal(err) }
You can just do require.NoError(t, sinkSrc.Close())
pkg/ccl/changefeedccl/sink_webhook_test.go, line 405 at r6 (raw file):
Quoted 4 lines of code…
sinkDestHost, err := url.Parse(sinkDest.URL()) if err != nil { t.Fatal(err) }
require.NoError?
Before, webhook sink requests simply resulted in retryable changefeed errors upon failure. This change adds default retry behavior for HTTP requests, preventing the need for changefeeds to shut down and restart every time. Resolves cockroachdb#67312 Release note: None
spiffyy99
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/sink_webhook.go, line 220 at r6 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
do you need to set this? Do we need such high precision?
reason we're setting this here is because the default MaxBackoff is 2 seconds. Thinking ahead to when we make this configurable, if users set a high initial backoff, they will expect to see max backoff higher than 2 seconds (depending on what they set).
0f051a3 to
0e5039e
Compare
|
bors r+ |
|
Build succeeded: |
Before, webhook sink requests simply resulted in retryable changefeed
errors upon failure. This change adds default retry behavior for HTTP
requests, preventing the need for changefeeds to shut down and restart
every time.
Resolves #67312
Release note: None