From 2537729913db244efe587d5aace67496b60a9020 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 22 Aug 2024 13:54:47 +1000 Subject: [PATCH] feat: allow zero retry directives eg: //ftl:retry 0 //ftl:retry 0 catch recover //ftl:retry 0 1s catch recover Not allowed: //ftl:retry 0 1s This allows for: - fsm transitions to opt out of the default fsm retry policy - having a catch verb without any retries --- backend/schema/metadataretry.go | 18 +++-- backend/schema/schema_test.go | 125 +++++++++++++++++++++++++++++++- backend/schema/validate.go | 7 +- backend/schema/validate_test.go | 3 +- 4 files changed, 138 insertions(+), 15 deletions(-) diff --git a/backend/schema/metadataretry.go b/backend/schema/metadataretry.go index d87acb4e5..505be1f01 100644 --- a/backend/schema/metadataretry.go +++ b/backend/schema/metadataretry.go @@ -109,17 +109,21 @@ func (m *MetadataRetry) RetryParams() (RetryParams, error) { // min backoff if m.MinBackoff == "" { - return RetryParams{}, fmt.Errorf("retry must have a minimum backoff") - } - minBackoff, err := parseRetryDuration(m.MinBackoff) - if err != nil { - return RetryParams{}, fmt.Errorf("could not parse min backoff duration: %w", err) + if params.Count != 0 { + return RetryParams{}, fmt.Errorf("retry must have a minimum backoff") + } + params.MinBackoff = MinBackoffLimit + } else { + minBackoff, err := parseRetryDuration(m.MinBackoff) + if err != nil { + return RetryParams{}, fmt.Errorf("could not parse min backoff duration: %w", err) + } + params.MinBackoff = minBackoff } - params.MinBackoff = minBackoff // max backoff if m.MaxBackoff == "" { - params.MaxBackoff = max(minBackoff, DefaultMaxBackoff) + params.MaxBackoff = max(params.MinBackoff, DefaultMaxBackoff) } else { maxBackoff, err := parseRetryDuration(m.MaxBackoff) if err != nil { diff --git a/backend/schema/schema_test.go b/backend/schema/schema_test.go index 82c110fb1..80e617e22 100644 --- a/backend/schema/schema_test.go +++ b/backend/schema/schema_test.go @@ -239,6 +239,7 @@ func TestParserRoundTrip(t *testing.T) { //nolint:maintidx func TestParsing(t *testing.T) { + zero := 0 ten := 10 tests := []struct { name string @@ -428,11 +429,23 @@ func TestParsing(t *testing.T) { +retry 1h1m5s verb C(Empty) Unit +retry 0h0m5s 1h0m0s - - fsm FSM { + verb D(Empty) Unit + +retry 0 + verb E(Empty) Unit + +retry 0 1s catch test.catch + verb F(Empty) Unit + +retry 0 catch test.catch + verb catch(builtin.CatchRequest) Unit + + fsm FSM + + retry 10 1s 10s + { start test.A transition test.A to test.B - transition test.A to test.C + transition test.B to test.C + transition test.C to test.D + transition test.D to test.E + transition test.E to test.F } } `, @@ -442,6 +455,13 @@ func TestParsing(t *testing.T) { Decls: []Decl{ &FSM{ Name: "FSM", + Metadata: []Metadata{ + &MetadataRetry{ + Count: &ten, + MinBackoff: "1s", + MaxBackoff: "10s", + }, + }, Start: []*Ref{ { Module: "test", @@ -462,13 +482,43 @@ func TestParsing(t *testing.T) { { From: &Ref{ Module: "test", - Name: "A", + Name: "B", }, To: &Ref{ Module: "test", Name: "C", }, }, + { + From: &Ref{ + Module: "test", + Name: "C", + }, + To: &Ref{ + Module: "test", + Name: "D", + }, + }, + { + From: &Ref{ + Module: "test", + Name: "D", + }, + To: &Ref{ + Module: "test", + Name: "E", + }, + }, + { + From: &Ref{ + Module: "test", + Name: "E", + }, + To: &Ref{ + Module: "test", + Name: "F", + }, + }, }, }, &Verb{ @@ -525,6 +575,73 @@ func TestParsing(t *testing.T) { }, }, }, + &Verb{ + Name: "D", + Request: &Ref{ + Module: "builtin", + Name: "Empty", + }, + Response: &Unit{ + Unit: true, + }, + Metadata: []Metadata{ + &MetadataRetry{ + Count: &zero, + }, + }, + }, + &Verb{ + Name: "E", + Request: &Ref{ + Module: "builtin", + Name: "Empty", + }, + Response: &Unit{ + Unit: true, + }, + Metadata: []Metadata{ + &MetadataRetry{ + Count: &zero, + MinBackoff: "1s", + Catch: &Ref{ + Module: "test", + Name: "catch", + }, + }, + }, + }, + &Verb{ + Name: "F", + Request: &Ref{ + Module: "builtin", + Name: "Empty", + }, + Response: &Unit{ + Unit: true, + }, + Metadata: []Metadata{ + &MetadataRetry{ + Count: &zero, + Catch: &Ref{ + Module: "test", + Name: "catch", + }, + }, + }, + }, + &Verb{ + Name: "catch", + Request: &Ref{ + Module: "builtin", + Name: "CatchRequest", + TypeParameters: []Type{ + &Any{}, + }, + }, + Response: &Unit{ + Unit: true, + }, + }, }, }}, }, diff --git a/backend/schema/validate.go b/backend/schema/validate.go index 07ea9ecef..e5be9b0e5 100644 --- a/backend/schema/validate.go +++ b/backend/schema/validate.go @@ -877,8 +877,8 @@ func validateVerbSubscriptions(module *Module, v *Verb, md *MetadataSubscriber, func validateRetries(module *Module, retry *MetadataRetry, requestType optional.Option[Type], scopes Scopes, schema optional.Option[*Schema]) (merr []error) { // Validate count - if retry.Count != nil && *retry.Count <= 0 { - merr = append(merr, errorf(retry, "retry count must be at least 1")) + if retry.Count != nil && *retry.Count < 0 { + merr = append(merr, errorf(retry, "retry count can not be negative")) } // Validate parsing of durations @@ -893,6 +893,9 @@ func validateRetries(module *Module, retry *MetadataRetry, requestType optional. // validate catch if retry.Catch == nil { + if retryParams.Count == 0 && retry.MinBackoff != "" { + merr = append(merr, errorf(retry, "can not define a backoff duration when retry count is 0 and no catch is declared")) + } return } req, ok := requestType.Get() diff --git a/backend/schema/validate_test.go b/backend/schema/validate_test.go index dc24531f4..9edb25f9b 100644 --- a/backend/schema/validate_test.go +++ b/backend/schema/validate_test.go @@ -380,9 +380,8 @@ func TestValidate(t *testing.T) { `19:7-7: could not parse min backoff duration: could not parse retry duration: duration has unknown unit "mins" - use 'd', 'h', 'm' or 's', eg '1d' or '30s'`, `21:7-7: max backoff duration (1s) needs to be at least as long as initial backoff (1m)`, `23:7-7: could not parse min backoff duration: retry backoff can not be larger than 1d`, - `25:7-7: retry count must be at least 1`, + `25:7-7: can not define a backoff duration when retry count is 0 and no catch is declared`, `30:7-7: catch can only be defined on verbs`, - `30:7-7: retry count must be at least 1`, `4:7-7: could not parse min backoff duration: could not parse retry duration: duration has unit "m" out of order - units need to be ordered from largest to smallest - eg '1d3h2m'`, `6:7-7: could not parse min backoff duration: could not parse retry duration: duration has unit "d" out of order - units need to be ordered from largest to smallest - eg '1d3h2m'`, `8:7-7: could not parse min backoff duration: retry must have a minimum backoff of 1s`,