-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up initial edge execution path #1158
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
@@ -578,26 +571,6 @@ func (e *executor) Execute(ctx context.Context, id state.Identifier, item queue. | |||
// we automatically enqueue all children of the dag from the root node. | |||
// This can be cleaned up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the comment need updating?
// GetRetries returns the number of retries for the function. | ||
func (f Function) GetRetries() int { | ||
// This uses retries in steps; a holdover from when steps were used in our DAG based approach. | ||
if len(f.Steps) == 0 || f.Steps[0].Retries == nil { | ||
return consts.DefaultRetryCount | ||
} | ||
return *f.Steps[0].Retries | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to the comment on retries -> attempts
, it might be better to encapsulate the + 1
inside this function and call it GetAttempts
, as we can clean up the retries/attempts mess when we add function.retries
in the config. After that:
- If SDK sends
step.retries.attempts
(legacy),+ 1
- If SDK sends
function.retries
(future), do not+ 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, good idea :)
Co-authored-by: Jack Williams <jack@inngest.com>
There are still some vestiges of our DAG-based days lurking around. This code simplifies the initial executor path, consolidating on the single-step approach we've been rolling with for a couple years — which won't change.
Type of change (choose one)
Checklist
Check our Pull Request Guidelines