Skip to content

feat: add maxRetry option #145

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: add maxRetry option #145

wants to merge 3 commits into from

Conversation

dongjiang1989
Copy link
Member

@volcano-sh-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign kevin-wangzefeng
You can assign the PR to them by writing /assign @kevin-wangzefeng in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dongjiang1989 dongjiang1989 force-pushed the add-global-maxretry-and-patch branch from c194290 to 46bf673 Compare November 29, 2024 06:15
@dongjiang1989 dongjiang1989 mentioned this pull request Nov 30, 2024
Open
googs1025
googs1025 reviewed Nov 30, 2024
View reviewed changes
googs1025
googs1025 reviewed Nov 30, 2024
View reviewed changes
@dongjiang1989 dongjiang1989 force-pushed the add-global-maxretry-and-patch branch from 291789f to 08fef71 Compare December 2, 2024 01:32
@dongjiang1989 dongjiang1989 mentioned this pull request Dec 2, 2024
Open
@hwdef
Copy link
Member

hwdef commented Dec 2, 2024

It doesn't look good. How about just adding a maxRetry field?

@hwdef
Copy link
Member

hwdef commented Dec 2, 2024 

type Flow struct {
	// +kubebuilder:validation:MinLength=1
	// +required
	Name string `json:"name"`
	// +optional
	DependsOn *DependsOn `json:"dependsOn,omitempty"`
	// +optional
	MaxRetry *MaxRetry `json:"maxRetry,omitempty"`
}

@dongjiang1989
add maxRetry and patch
615f915 
Signed-off-by: dongjiang <[email protected]>
@dongjiang1989 dongjiang1989 force-pushed the add-global-maxretry-and-patch branch from cebc0eb to 615f915 Compare December 3, 2024 07:52
@dongjiang1989
Copy link
Member Author

It doesn't look good. How about just adding a maxRetry field?

Thanks @hwdef @googs1025
Got it. Just add maxRetry field done.

@dongjiang1989
Copy link
Member Author

@googs1025 Please re-check it

@googs1025
Copy link
Member

feel free to go ahead.
small suggestion: I like to fully express the meaning of this field on the field, just like the API docs maintained in k/k, which make it easier for users to understand the meaning of each field.

@googs1025
Copy link
Member

@hwdef @Monokaix @JesseStutler /PTAL

@hwdef
Copy link
Member

hwdef commented Feb 24, 2025

/lgtm

@dongjiang1989
Copy link
Member Author

cc @googs1025 @Monokaix
Please re-check it

@dongjiang1989 dongjiang1989 mentioned this pull request Mar 17, 2025
Open
@dongjiang1989 dongjiang1989 changed the title feat: add maxRetry and patch option feat: add maxRetry option Mar 17, 2025
@dongjiang1989
Copy link
Member Author

dongjiang1989 commented Mar 18, 2025 •
edited
Loading

@Monokaix Please review it. Thanks
Similar to this PR, when a CRD field changes, how to depend on it in volcano and submit a volcano PR?

Monokaix
Monokaix reviewed Mar 19, 2025
View reviewed changes
pkg/apis/flow/v1alpha1/jobflow_types.go Outdated
@@ -44,6 +44,12 @@ type Flow struct {
Name string `json:"name"`
// +optional
DependsOn *DependsOn `json:"dependsOn,omitempty"`
// Defaults to 1.
Copy link
Member

Choose a reason for hiding this comment

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

Why not 3 and be consistent with vc job's default value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @Monokaix
Got it
Default setting 3

@Monokaix
Copy link
Member

feel free to go ahead. small suggestion: I like to fully express the meaning of this field on the field, just like the API docs maintained in k/k, which make it easier for users to understand the meaning of each field.

Yeah I agree, comments should be added, and will this value be passed to the maxretry of vcjob?

@dongjiang1989
Copy link
Member Author

feel free to go ahead. small suggestion: I like to fully express the meaning of this field on the field, just like the API docs maintained in k/k, which make it easier for users to understand the meaning of each field.

Yeah I agree, comments should be added, and will this value be passed to the maxretry of vcjob?

Yeah. Pass this value to the maxretry of vcjob, replace the default value in jobTemplate

@volcano-sh-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Monokaix Monokaix Monokaix left review comments

@googs1025 googs1025 googs1025 left review comments

@k82cn k82cn Awaiting requested review from k82cn

@wpeng102 wpeng102 Awaiting requested review from wpeng102

Assignees

@hwdef hwdef

Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dongjiang1989 @volcano-sh-bot @hwdef @googs1025 @Monokaix