feat: add retry directive #1552
Conversation
matt2e
requested review from
a team and
wesbillman
and removed request for
a team
| schemapb "github.com/TBD54566975/ftl/backend/protos/xyz/block/ftl/v1/schema" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
String constants are just to make error printing have the same format we expect users to use (ie 1d not go's standard 24h0m0s)
| } | ||
| } | ||
| `, | ||
| errs: []string{ |
There was a problem hiding this comment.
Please read these to see if they are clear
matt2e
force-pushed
the
matt2e/retry-directives
branch
2 times, most recently
from
8398f38 to
2196d1d
Compare
backend/schema/metadataretry.go
Outdated
|
|
||
| Count *int `parser:"'+' 'retry' (@Number Whitespace)?" protobuf:"2,optional"` | ||
| MinBackoff string `parser:"@(Number Ident)?" protobuf:"3"` | ||
| MaxBackoff string `parser:"@(Number Ident)?" protobuf:"4"` |
There was a problem hiding this comment.
FYI, the reasoning behind @(Number Ident)? is that I could not find a sane way to parse multiple number + unit pairs while ensuring no whitespace inbetween
Also, 1m30s is tokenized as 1 (number) + m30s (ident).
Instead we are just getting these in as strings and then parsing them ourselves using regex. This helps us customize the error messages for more cases.
backend/schema/metadataretry.go
Outdated
| count := int64(*m.Count) | ||
| return &schemapb.MetadataRetry{ | ||
| Pos: posToProto(m.Pos), | ||
| Count: &count, |
There was a problem hiding this comment.
You should be able to use proto.Int64(int64(m.Count)) here.
There was a problem hiding this comment.
Oh, looks like i made a more fundamental mistake... not handling m.Count == 0
Updated to
var count *int64
if m.Count != nil {
count = proto.Int64(int64(*m.Count))
}| merr = append(merr, errorf(md, "verb %s: %v", n.Name, err)) | ||
| return | ||
| } | ||
| if maxDuration, ok := maxDuration.Get(); ok && maxDuration < minDuration { |
There was a problem hiding this comment.
Should there be a check against MaxBackoffDuration (ie. 24h)?
There was a problem hiding this comment.
Both MinBackoff and MaxBackoff are checked for min/max during parseRetryDuration
| `25:7-7: verb K: retry count must be atleast 1`, | ||
| `4:7-7: verb A: retry has unit "m" out of order. Units need to be ordered from largest to smallest`, | ||
| `6:7-7: verb B: retry has unit "d" out of order. Units need to be ordered from largest to smallest`, | ||
| `8:7-7: verb C: retry must have a minimum backoff of 1s`, |
matt2e
force-pushed
the
matt2e/retry-directives
branch
from
a542225 to
111b0ff
Compare
adds retry directive for fsm transition verbs. This PR does not contain logic to execute retries.
//ftl:retry [count] minbackoff [maxbackoff]+retry [count] minbackoff [maxbackoff]s,m,h,dunits like90sor1m30s1s/1d(very debatable...)