-
Notifications
You must be signed in to change notification settings - Fork 321
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
[RayJob][Status][5/n] Refactor getOrCreateK8sJob #1750
[RayJob][Status][5/n] Refactor getOrCreateK8sJob #1750
Conversation
|
||
// Check the status of the k8s job and update the RayJobInstance status accordingly. | ||
// Get the k8s job | ||
k8sJob := &batchv1.Job{} |
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.
We don't need to Get
the K8s Job again because getOrCreateK8sJob
has already performed this check.
return ctrl.Result{RequeueAfter: RayJobDefaultRequeueDuration}, err | ||
} | ||
|
||
if wasJobCreated { |
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.
Print the messages immediately after the job is created instead.
@@ -262,17 +241,16 @@ func (r *RayJobReconciler) Reconcile(ctx context.Context, request ctrl.Request) | |||
// the RayJob is submitted against the RayCluster created by THIS job, then | |||
// try to gracefully stop the Ray job and delete (suspend) the cluster | |||
if rayJobInstance.Spec.Suspend && len(rayJobInstance.Spec.ClusterSelector) == 0 { | |||
info, err := rayDashboardClient.GetJobInfo(ctx, rayJobInstance.Status.JobId) |
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.
We don't need to have an additional GetJobInfo.
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.
Good catch!
e7a9ec6
to
fde6271
Compare
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.
Nice!
Co-authored-by: Archit Kulkarni <architkulkarni@users.noreply.github.com> Signed-off-by: Kai-Hsun Chen <kaihsun@apache.org>
Why are these changes needed?
Related issue number
Checks