Add more generic error predicates #3116

emilymye · 2020-02-12T02:29:05Z

Added check for Temporary() error
Added check for connection reset by peer error

Fixes hashicorp/terraform-provider-google#3957

Release Note Template for Downstream PRs (will be copied)

Added retries for common network errors we've encountered.

modular-magician · 2020-02-12T02:33:44Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 28 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 1 file changed, 28 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 1 file changed, 28 insertions(+), 4 deletions(-))

modular-magician · 2020-02-12T04:47:24Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 32 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 1 file changed, 32 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 1 file changed, 32 insertions(+), 4 deletions(-))

emilymye · 2020-02-12T18:45:22Z

reviewers - I think it's not a bad idea to add the Temporary check, but since we don't currently do this, I'm also fine removing it for now.

rileykarson

I'm just curious what would implement temporary. Is that a standard thing for temporary errors to implement, and if it is, can we link to docs or something on it from our interface?

rileykarson · 2020-02-12T20:17:07Z

third_party/terraform/utils/error_retry_predicates.go

-	if urlerr, ok := err.(*url.Error); ok && urlerr.Timeout() {
-		log.Printf("[DEBUG] Dismissed an error as retryable based on googleapis.com target: %s", err)
+type temporary interface {
+	Temporary() bool


Does anything implement this right now?

yup - url.Error and net.OpError - I actually noticed that some of the Go client libraries did this (like googleapis/google-cloud-go@8007d26)

Thanks! Mind commenting and linking to an example of an implementer?

Also, I think we actually want to call Temporary(), right? Since they'll return false for non-temporary errors based on https://golang.org/src/net/net.go?s=13212:13333#L384

i realized right after and pushed an update ha

rileykarson · 2020-02-12T20:20:15Z

third_party/terraform/utils/error_retry_predicates.go

+	return false, ""
+}
+
+func isRetryableUrlError(err error) (bool, string) {


(non-blocking) I have a slight preference for implementing this as many smaller predicates, but it isn't too strongly held.

:+1 am fine with that too. Thoughts on retrying on all net.OpErrors in general?

I'm curious what the actual errors returned by that package end up looking like, if you think it would work I'm 👍. At the very least, errors for whom Temporary() and Timeout() return true make sense based on https://golang.org/src/net/net.go?s=13212:13333#L384.

rileykarson · 2020-02-13T00:27:42Z

third_party/terraform/utils/error_retry_predicates.go

-	if urlerr, ok := err.(*url.Error); ok && urlerr.Timeout() {
-		log.Printf("[DEBUG] Dismissed an error as retryable based on googleapis.com target: %s", err)
+type temporary interface {
+	Temporary() bool


Thanks! Mind commenting and linking to an example of an implementer?

Also, I think we actually want to call Temporary(), right? Since they'll return false for non-temporary errors based on https://golang.org/src/net/net.go?s=13212:13333#L384

rileykarson · 2020-02-13T00:31:38Z

third_party/terraform/utils/error_retry_predicates.go

+	return false, ""
+}
+
+func isRetryableUrlError(err error) (bool, string) {


I'm curious what the actual errors returned by that package end up looking like, if you think it would work I'm 👍. At the very least, errors for whom Temporary() and Timeout() return true make sense based on https://golang.org/src/net/net.go?s=13212:13333#L384.

modular-magician · 2020-02-13T00:34:54Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 43 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 1 file changed, 43 insertions(+), 1 deletion(-))
TF Conversion: Diff ( 1 file changed, 43 insertions(+), 1 deletion(-))

modular-magician · 2020-02-13T00:34:57Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 43 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 1 file changed, 43 insertions(+), 1 deletion(-))
TF Conversion: Diff ( 1 file changed, 43 insertions(+), 1 deletion(-))

modular-magician · 2020-02-13T19:07:33Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 57 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 1 file changed, 57 insertions(+), 6 deletions(-))
TF Conversion: Diff ( 1 file changed, 57 insertions(+), 6 deletions(-))

modular-magician · 2020-02-13T19:08:57Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 57 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 1 file changed, 57 insertions(+), 6 deletions(-))
TF Conversion: Diff ( 1 file changed, 57 insertions(+), 6 deletions(-))

modular-magician · 2020-02-13T19:22:29Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 56 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 1 file changed, 56 insertions(+), 6 deletions(-))
TF Conversion: Diff ( 1 file changed, 56 insertions(+), 6 deletions(-))

* add connection reset by peer predicate * lint * separate error preds, fix temp pred * Fix error retries, comments * remove removed func

googlebot added the cla: yes label Feb 12, 2020

emilymye requested review from rileykarson and danawillow February 12, 2020 18:44

rileykarson reviewed Feb 12, 2020

View reviewed changes

emilymye force-pushed the retryconnectionreset branch from 526c8ee to eed5d3e Compare February 13, 2020 00:30

rileykarson approved these changes Feb 13, 2020

View reviewed changes

emilymye added 4 commits February 13, 2020 11:02

add connection reset by peer predicate

4275120

lint

1a23aee

separate error preds, fix temp pred

33e160d

Fix error retries, comments

f0b7d22

emilymye force-pushed the retryconnectionreset branch from cd13777 to f0b7d22 Compare February 13, 2020 19:03

remove removed func

819594f

emilymye merged commit 4033f0b into GoogleCloudPlatform:master Feb 13, 2020

This was referenced Feb 13, 2020

Add more generic error predicates hashicorp/terraform-provider-google#5675

Merged

Add more generic error predicates hashicorp/terraform-provider-google-beta#1762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more generic error predicates #3116

Add more generic error predicates #3116

emilymye commented Feb 12, 2020 •

edited

Loading

modular-magician commented Feb 12, 2020

modular-magician commented Feb 12, 2020

emilymye commented Feb 12, 2020

rileykarson left a comment

rileykarson Feb 12, 2020

emilymye Feb 13, 2020 •

edited

Loading

rileykarson Feb 13, 2020

emilymye Feb 13, 2020

rileykarson Feb 12, 2020 •

edited

Loading

emilymye Feb 13, 2020

rileykarson Feb 13, 2020

rileykarson Feb 13, 2020

rileykarson Feb 13, 2020

modular-magician commented Feb 13, 2020

modular-magician commented Feb 13, 2020

modular-magician commented Feb 13, 2020

modular-magician commented Feb 13, 2020

modular-magician commented Feb 13, 2020

Add more generic error predicates #3116

Add more generic error predicates #3116

Conversation

emilymye commented Feb 12, 2020 • edited Loading

modular-magician commented Feb 12, 2020

Diff report:

modular-magician commented Feb 12, 2020

Diff report:

emilymye commented Feb 12, 2020

rileykarson left a comment

Choose a reason for hiding this comment

rileykarson Feb 12, 2020

Choose a reason for hiding this comment

emilymye Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

rileykarson Feb 13, 2020

Choose a reason for hiding this comment

emilymye Feb 13, 2020

Choose a reason for hiding this comment

rileykarson Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

emilymye Feb 13, 2020

Choose a reason for hiding this comment

rileykarson Feb 13, 2020

Choose a reason for hiding this comment

rileykarson Feb 13, 2020

Choose a reason for hiding this comment

rileykarson Feb 13, 2020

Choose a reason for hiding this comment

modular-magician commented Feb 13, 2020

Diff report:

modular-magician commented Feb 13, 2020

Diff report:

modular-magician commented Feb 13, 2020

Diff report:

modular-magician commented Feb 13, 2020

Diff report:

modular-magician commented Feb 13, 2020

Diff report:

emilymye commented Feb 12, 2020 •

edited

Loading

emilymye Feb 13, 2020 •

edited

Loading

rileykarson Feb 12, 2020 •

edited

Loading