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.

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

[YUNIKORN-268] When deleting a deployment the application is not deleted from the shim cache #155

Merged
merged 6 commits into from
Jul 17, 2020
Merged

[YUNIKORN-268] When deleting a deployment the application is not deleted from the shim cache #155

merged 6 commits into from
Jul 17, 2020

Conversation

HuangTing-Yao
Copy link
Contributor

improve RemoveApplication() and TestRemoveApplication()

@HuangTing-Yao
Copy link
Contributor Author

@yangwwei Please help to review. Thanks.

yangwwei
yangwwei requested changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@yangwwei yangwwei left a comment

Choose a reason for hiding this comment

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

hi @HuangTing-Yao thanks for the PR. overall it looks good. See my comments and let me know if that makes sense or not. Thanks.

pkg/cache/context.go Show resolved Hide resolved
pkg/cache/context.go Outdated Show resolved Hide resolved
pkg/cache/context.go Outdated Show resolved Hide resolved
pkg/cache/context_test.go Outdated Show resolved Hide resolved
pkg/cache/context_test.go Show resolved Hide resolved
pkg/common/si_helper.go Show resolved Hide resolved
yangwwei
yangwwei requested changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@yangwwei yangwwei left a comment

Choose a reason for hiding this comment

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

hi @HuangTing-Yao

It mostly looks good, I just add another comment to refactor the code a bit. Rest looks good to me. @kingamarton pls also take a look at the changes. Thanks!

pkg/cache/context.go Outdated Show resolved Hide resolved
yangwwei
yangwwei requested changes Jul 16, 2020
View reviewed changes
pkg/cache/application.go Show resolved Hide resolved
yangwwei
yangwwei approved these changes Jul 16, 2020
View reviewed changes
Copy link
Contributor

@yangwwei yangwwei left a comment

Choose a reason for hiding this comment

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

LGTM.

kingamarton
kingamarton requested changes Jul 16, 2020
View reviewed changes
Copy link
Contributor

@kingamarton kingamarton left a comment

Choose a reason for hiding this comment

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

Have you checked if the application is removed from the partition as well in the core side?

pkg/cache/application_test.go Outdated Show resolved Hide resolved
pkg/cache/application_test.go Outdated Show resolved Hide resolved
pkg/cache/application_test.go Outdated Show resolved Hide resolved
pkg/cache/context_test.go Outdated Show resolved Hide resolved
pkg/shim/scheduler_test.go Outdated Show resolved Hide resolved
@HuangTing-Yao
Copy link
Contributor Author

@kingamarton this PR only handle the update request should send to core.
Doesn't check the core part.

@yangwwei
Copy link
Contributor

We can do this incrementally. We don't have to check the core side changes in this PR, it's fine to do that later when we hook things up together. The core side removeApp has UT covered, that should work. The changes in this PR looks good, I'll merge shortly.

@yangwwei yangwwei merged commit 97db743 into apache:master Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kingamarton kingamarton kingamarton requested changes

@yangwwei yangwwei yangwwei approved these changes

Assignees

@HuangTing-Yao HuangTing-Yao

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@HuangTing-Yao @yangwwei @kingamarton