Skip to content
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

add UpdateFunc to handle update events #313

Merged
merged 1 commit into from
Jan 15, 2018
Merged

Conversation

mqliang
Copy link
Contributor

@mqliang mqliang commented Jan 15, 2018

  • Add an Update function to the controller.
  • The informer periodically generates an Update event but we aren't processing these because we don't have an update function.
  • As a result, TrainingJob.reconcile doesn't get called periodically and we aren't properly updating the status of the job.

ref #309
ref #293

This change is Reviewable

@coveralls
Copy link

coveralls commented Jan 15, 2018

Coverage Status

Coverage increased (+0.05%) to 31.664% when pulling 2383700 on mqliang:resync into 98a34a1 on tensorflow:master.

@jlewi
Copy link
Contributor

jlewi commented Jan 15, 2018

Thanks for this PR; unfortunately this is blocked on fixing head #293.

In the meantime could you update the PR description to explain in more detail what this PR is addressing?

@jlewi
Copy link
Contributor

jlewi commented Jan 15, 2018

LGTM. I'm going to merge this PR so I can merge it into #308.
I'll fix the lint error in that PR; doesn't look like this PR is what caused the lint error which is what's causing travis build to fail.

@jlewi jlewi merged commit 2109be9 into kubeflow:master Jan 15, 2018
@mqliang mqliang deleted the resync branch January 15, 2018 22:00
@gaocegege
Copy link
Member

I am not sure if updatefunc for TFJob works, since I think updatefuncs for job and service are also necessary to update the TFJob status 🤔

WDYT @mqliang

@mqliang
Copy link
Contributor Author

mqliang commented Jan 16, 2018

@gaocegege
Yes, you are right. We should also watch events for jobs/services and process their corresponding TFJobs.

I will send a PR to address this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants