Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Adding flags for ignore-retry-cause and default-max-attempts #596

Closed
wants to merge 13 commits into from
Closed

Adding flags for ignore-retry-cause and default-max-attempts #596

wants to merge 13 commits into from

Conversation

hamersaw
Copy link
Contributor

@hamersaw hamersaw commented Jul 26, 2023 •
edited
Loading

TL;DR

Adding flags to simplify retry behavior. ignore-retry-cause means that system and user failures are treated the same and default-max-attempts sets a default number of retries for tasks.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

^^^

Tracking Issue

NA

Follow-up issue

NA

@hamersaw
added flags for ignore-retry-cause and default-max-attempts
b201c6b 
Signed-off-by: Daniel Rammer <daniel@union.ai>
@fellhorn fellhorn mentioned this pull request Aug 23, 2023
Merged
8 tasks
@fg91 fg91 mentioned this pull request Sep 10, 2023
Merged
3 tasks
@hamersaw
Merge branch 'master' into feature/simplify-retries
060ff3a 
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw hamersaw marked this pull request as ready for review September 14, 2023 14:34
fellhorn and others added 3 commits September 14, 2023 09:35
@fellhorn
Switch non interruptible @ unified retry behavior (#610)
56eb391 
Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>
@hamersaw
fixed lint issues
6c91faa 
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw
began unit tests
210975c 
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw
Copy link
Contributor Author

hamersaw commented Sep 14, 2023 •
edited
Loading

In the current implementation enabling ignoreRetryCause also turns the interruptibleFailureThreshold configuration into a complementary value. IMO this will result in unintended behavior and should be handled separately. Unit tests will need to be completed / updated accordingly.

@hamersaw hamersaw mentioned this pull request Sep 14, 2023
Merged
8 tasks
@hamersaw
enabling negative interruptibleFailureThreshold
b754c3f 
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw
fixed unit tests
6a73b29 
Signed-off-by: Daniel Rammer <daniel@union.ai>
@codecov
Copy link

codecov bot commented Sep 14, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@0fcc1da). Click here to learn what that means.
The diff coverage is 91.42%.

❗ Current head 1af296d differs from pull request most recent head 6af5f8f. Consider uploading reports for the commit 6af5f8f to get more accurate results

Additional details and impacted files

fellhorn and others added 6 commits September 29, 2023 09:39
@fellhorn
Finish simplify retry behavior PR (#623)
a9b52af 
* Cleanup retry behavior

Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>

* Fix interruptible retry threshold for odl behavior

Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>

* Add tests for BuildNodeExecutionContext

Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>

* Fix IsElgibileForRetries Tests

Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>

---------

Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>
@hamersaw
Merge branch 'master' into feature/simplify-retries
397120b 
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw
updated flyteplugins
d0b7b27 
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw
Merge branch 'master' into feature/simplify-retries
b728529 
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw
fixed lint issue
f919ad6 
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw
Merge branch 'master' into feature/simplify-retries
6af5f8f 
Signed-off-by: Daniel Rammer <daniel@union.ai>
pingsutw
pingsutw reviewed Sep 29, 2023
View reviewed changes
pkg/controller/nodes/node_exec_context.go
// interruptible machine
maxAttempts := uint32(config.GetConfig().NodeConfig.DefaultMaxAttempts)
if n.GetRetryStrategy() != nil && n.GetRetryStrategy().MinAttempts != nil && *n.GetRetryStrategy().MinAttempts != 1 {
maxAttempts = uint32(*n.GetRetryStrategy().MinAttempts)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we deprecate MinAttempts and add MaxAttempts 😂

@eapolinario
Copy link
Contributor

Hi, we are moving all Flyte development to a monorepo. In order to help the transition period, we're moving open PRs to monorepo automatically and your PR was moved to flyteorg/flyte#4153. Notice that if there are any conflicts in the resulting PR they most likely happen due to the change in the import path of the flyte components.

@eapolinario eapolinario closed this Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fg91 fg91 fg91 left review comments

@fellhorn fellhorn fellhorn left review comments

@pingsutw pingsutw pingsutw approved these changes

@kumare3 kumare3 Awaiting requested review from kumare3 kumare3 is a code owner

@EngHabu EngHabu Awaiting requested review from EngHabu EngHabu is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hamersaw @eapolinario @fellhorn @fg91 @pingsutw