Retain for job and metricsCollector #572
Conversation
|
Fixes: #565 |
|
/test kubeflow-katib-presubmit |
| @@ -245,6 +240,9 @@ func (r *ReconcileTrial) reconcileJob(instance *trialsv1alpha2.Trial, desiredJob | |||
| err = r.Get(context.TODO(), types.NamespacedName{Name: desiredJob.GetName(), Namespace: desiredJob.GetNamespace()}, deployedJob) | |||
| if err != nil { | |||
| if errors.IsNotFound(err) { | |||
| if instance.IsCompleted() { | |||
There was a problem hiding this comment.
Which condition is being targeted here?
There was a problem hiding this comment.
func (trial *Trial) IsCompleted() bool {
return trial.IsSucceeded() || trial.IsFailed() || trial.IsKilled()
}
There was a problem hiding this comment.
Can you add a comment here?
There was a problem hiding this comment.
https://github.com/kubeflow/katib/blob/master/pkg/controller/v1alpha2/trial/trial_controller.go#L149 returns if instance is completed.
There was a problem hiding this comment.
do you mean add a comment like IsCompleted() targets "IsSucceeded() || IsFailed() || IsKilled()" everywhere IsCompleted() is called?
There was a problem hiding this comment.
Line 147~151 deleted in this PR
There was a problem hiding this comment.
Sorry. I missed that change. Can you explain the difference? And what I meant, is to add a comment why this check is added. Readers will find it easier to follow the code.
|
/test kubeflow-katib-presubmit |
|
/lgtm |
|
/test kubeflow-katib-presubmit |
4 similar comments
|
/test kubeflow-katib-presubmit |
|
/test kubeflow-katib-presubmit |
|
/test kubeflow-katib-presubmit |
|
/test kubeflow-katib-presubmit |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hougangliu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This change is