Add client side backoff retries for pubsub messages #1218

yolocs · 2020-06-04T18:48:32Z

Additional Changes

added handler constructor to initialize new fields
removed pool package as it feels a bit unnature to have a deeper package pkg/broker/handler/pool depend on a parent package pkg/broker/handler implementation; no functional change involved

Release Note

Add client side backoff retries for pubsub messages

Docs

yolocs · 2020-06-04T20:00:25Z

/retest

yolocs · 2020-06-04T20:30:08Z

/retest

pkg/broker/handler/fanout_test.go

pkg/broker/handler/handler_test.go

pkg/broker/handler/handler.go

pkg/broker/handler/handler_test.go

cmd/broker/retry/main.go

cmd/broker/fanout/main.go

cmd/broker/retry/main.go

pkg/broker/handler/handler_test.go

ian-mi

It looks like a retry policy is not yet set anywhere?

pkg/broker/handler/handler.go

liu-cong · 2020-06-08T18:55:59Z

cmd/broker/retry/main.go

@@ -58,6 +58,9 @@ type envConfig struct {

 	// Max to 10m.
 	TimeoutPerEvent time.Duration `envconfig:"TIMEOUT_PER_EVENT"`
+
+	MinRetryBackoff time.Duration `envconfig:"MIN_RETRY_BACKOFF" default:"1s"`
+	MaxRetryBackoff time.Duration `envconfig:"MAX_RETRY_BACKOFF" default:"30s"`


I was wondering if this should be a bit longer, like a few minutes?

How long would you suggest? I don't have a preference. I simply don't want to over blocking the queue because of the limitation we have today.

Changed to 1m now.

I just think if the backoff reaches 30s, it's very likely the issue is not transient, and some longer backoff is probably better.

But just a guess. We can always tune this later. 1min seems good to me.

liu-cong · 2020-06-08T20:16:56Z

/lgtm
/approve

knative-prow-robot · 2020-06-08T20:17:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liu-cong, yolocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [liu-cong,yolocs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yolocs · 2020-06-08T20:44:00Z

/retest

yolocs · 2020-06-08T21:44:32Z

/retest

knative-metrics-robot · 2020-06-08T22:41:35Z

The following is the coverage report on the affected files.
Say /test pull-google-knative-gcp-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/broker/handler/fanout.go	Do not exist	88.4%
pkg/broker/handler/handler.go	82.9%	84.6%	1.8
pkg/broker/handler/options.go	Do not exist	88.9%
pkg/broker/handler/pool.go	Do not exist	82.1%
pkg/broker/handler/providers.go	Do not exist	60.0%
pkg/broker/handler/retry.go	Do not exist	86.4%
pkg/broker/handler/wire_gen.go	Do not exist	75.0%

liu-cong · 2020-06-08T22:53:28Z

/lgtm

nachocano · 2020-06-09T01:03:32Z

/retest

…

On Mon, Jun 8, 2020 at 4:06 PM Knative test reporter robot < ***@***.***> wrote: The following jobs failed: Test name Triggers Retries pull-google-knative-gcp-integration-tests 0/3 Failed non-flaky tests preventing automatic retry of pull-google-knative-gcp-integration-tests: github.com/google/knative-gcp/test/e2e.TestEventTransformationForSubscriptiongithub.com/google/knative-gcp/test/e2e.TestChannelChaingithub.com/google/knative-gcp/test/e2e.TestChannelTracinggithub.com/google/knative-gcp/test/e2e.TestChannelTracing/includes_incoming_trace_idgithub.com/google/knative-gcp/test/e2e.TestChannelChain/Channel-messaging.cloud.google.com/v1alpha1github.com/google/knative-gcp/test/e2e.TestChannelChain/Channel-messaging.cloud.google.com/v1beta1github.com/google/knative-gcp/test/e2e.TestEventTransformationForSubscription/Channel-messaging.cloud.google.com/v1alpha1github.com/google/knative-gcp/test/e2e.TestEventTransformationForSubscription/Channel-messaging.cloud.google.com/v1beta1 — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1218 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABD65DBQJSIL7PXZVJBFB33RVVVGHANCNFSM4NS4ONSA> .

knative-test-reporter-robot · 2020-06-09T02:30:28Z

The following jobs failed:

Test name	Triggers	Retries
pull-google-knative-gcp-integration-tests		0/3
pull-google-knative-gcp-wi-tests	pull-google-knative-gcp-wi-tests pull-google-knative-gcp-wi-tests	2/3

Automatically retrying due to test flakiness...
/test pull-google-knative-gcp-wi-tests

yolocs added 3 commits June 4, 2020 16:13

Temporary implementation of backoff retry

e884ad1

Merge branch 'master' into f/slowretry

46ae327

refactor

acc5c28

yolocs requested review from grantr, nachocano and liu-cong June 4, 2020 18:48

knative-prow-robot requested review from Harwayne and ian-mi June 4, 2020 18:48

knative-prow-robot added the approved label Jun 4, 2020

googlebot added the cla: yes (override cla status due to multiple authors bug) label Jun 4, 2020

knative-prow-robot added needs-rebase size/L labels Jun 4, 2020

yolocs removed request for Harwayne and ian-mi June 4, 2020 18:50

resolve conflict

09db5b2

knative-prow-robot added needs-rebase and removed needs-rebase labels Jun 4, 2020

yolocs added 2 commits June 4, 2020 18:55

Merge branch 'master' into f/slowretry

604a01e

resolve conflicts

8ae2315

knative-prow-robot removed the needs-rebase label Jun 4, 2020

liu-cong reviewed Jun 5, 2020

View reviewed changes

pkg/broker/handler/fanout_test.go Outdated Show resolved Hide resolved

liu-cong reviewed Jun 5, 2020

View reviewed changes

pkg/broker/handler/handler_test.go Show resolved Hide resolved

liu-cong reviewed Jun 5, 2020

View reviewed changes

ian-mi reviewed Jun 5, 2020

View reviewed changes

pkg/broker/handler/handler.go Outdated Show resolved Hide resolved

yolocs added 2 commits June 8, 2020 16:40

Merge branch 'master' into f/slowretry

2731c03

comments

9f59c15

liu-cong reviewed Jun 8, 2020

View reviewed changes

to 1m

27b559e

knative-prow-robot assigned liu-cong Jun 8, 2020

knative-prow-robot added the lgtm label Jun 8, 2020

Merge branch 'master' into f/slowretry

dbfff2f

knative-prow-robot removed the lgtm label Jun 8, 2020

knative-prow-robot added the lgtm label Jun 8, 2020

knative-prow-robot merged commit 5556846 into google:master Jun 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add client side backoff retries for pubsub messages #1218

Add client side backoff retries for pubsub messages #1218

yolocs commented Jun 4, 2020 •

edited

Loading

yolocs commented Jun 4, 2020

yolocs commented Jun 4, 2020

ian-mi left a comment

liu-cong Jun 8, 2020

yolocs Jun 8, 2020

yolocs Jun 8, 2020

liu-cong Jun 8, 2020

liu-cong commented Jun 8, 2020

knative-prow-robot commented Jun 8, 2020

yolocs commented Jun 8, 2020

yolocs commented Jun 8, 2020

knative-metrics-robot commented Jun 8, 2020

liu-cong commented Jun 8, 2020

nachocano commented Jun 9, 2020 via email

knative-test-reporter-robot commented Jun 9, 2020

Add client side backoff retries for pubsub messages #1218

Add client side backoff retries for pubsub messages #1218

Conversation

yolocs commented Jun 4, 2020 • edited Loading

Additional Changes

yolocs commented Jun 4, 2020

yolocs commented Jun 4, 2020

ian-mi left a comment

Choose a reason for hiding this comment

liu-cong Jun 8, 2020

Choose a reason for hiding this comment

yolocs Jun 8, 2020

Choose a reason for hiding this comment

yolocs Jun 8, 2020

Choose a reason for hiding this comment

liu-cong Jun 8, 2020

Choose a reason for hiding this comment

liu-cong commented Jun 8, 2020

knative-prow-robot commented Jun 8, 2020

yolocs commented Jun 8, 2020

yolocs commented Jun 8, 2020

knative-metrics-robot commented Jun 8, 2020

liu-cong commented Jun 8, 2020

nachocano commented Jun 9, 2020 via email

knative-test-reporter-robot commented Jun 9, 2020

yolocs commented Jun 4, 2020 •

edited

Loading