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
⚠ Support shutdown watches dynamically (v2) #2159
base: main
Are you sure you want to change the base?
Conversation
Hi @inteon. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
Thanks @inteon , nice work!
It seems you have removed the Stop()
from Controller, so how should users stop one or more specific controller?
/ok-to-test |
@FillZpp what do you think about a more generic solution for canceling runnables: // NewCancelRunnable returns a new Runnable and a function to cancel that runnable.
func NewCancelRunnable(r Runnable) (Runnable, context.CancelFunc) {
mu := sync.Mutex{}
cancel := (func())(nil)
canceled := false
return RunnableFunc(func(ctx context.Context) error {
mu.Lock()
ctx, cancel = context.WithCancel(ctx)
if canceled {
cancel()
}
mu.Unlock()
return r.Start(ctx)
}), func() {
mu.Lock()
defer mu.Unlock()
canceled = true
if cancel != nil {
cancel()
}
}
} |
Emm, I think it's alright. It may not be intuitive as the Stop method, but more generic as you said. |
/label tide/merge-method-squash |
fixes #1884 |
da38baf
to
57edf50
Compare
f57dd5d
to
a169245
Compare
cbddc27
to
12b0e37
Compare
@inteon Thank you for your contribution with this PR. I've noticed that this PR seems to have not been updated for a while. I believe this PR is very important for the enhancement of this project, and I also have use cases for this feature. If there is any way I can help, such as conducting code reviews or assisting with testing, I would be more than happy to contribute. |
@lou-lan I'm not sure what is blocking this PR. |
Sorry, I don't understand this change. Contrary to what the title claims, it never stops any watch, it stops the source. How does that help? Please provide a real-world use-case. If this is supposed to work towards allowing to stop watches by refcounting if they have eventhandlers or are otherwise used, I'd like to see a description somewhere as to how exactly this will work once all pieces are done. |
12b0e37
to
798737f
Compare
eb6f3e2
to
6c16925
Compare
@alvaroaleman I just added a |
/test pull-controller-runtime-test |
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
093d2ce
to
cc38429
Compare
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Co-authored-by: FillZpp <FillZpp.pub@gmail.com> Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
cc38429
to
b50828a
Compare
/retest |
@inteon: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
PTAL at the original PR: #2099
I started making some small changes to the original PR, thinking there would be an easy fix. However, it turned out that there were still a lot of fundamental questions requiring more time and effort to get fixed.
The current state of the PR is best described by #2159 (comment).
(4/4) PR chain: