-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Data mover micro-service watcher #7999
Data mover micro-service watcher #7999
Conversation
475eac6
to
a80e88b
Compare
a80e88b
to
03cf952
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7999 +/- ##
==========================================
+ Coverage 58.82% 58.88% +0.06%
==========================================
Files 346 351 +5
Lines 28891 29367 +476
==========================================
+ Hits 16996 17294 +298
- Misses 10463 10639 +176
- Partials 1432 1434 +2 ☔ View full report in Codecov by Sentry. |
03cf952
to
81fcb63
Compare
81fcb63
to
b0bfd03
Compare
I think re-titling |
Done |
|
||
defer func() { | ||
if !succeeded { | ||
if err := eventInformer.RemoveEventHandler(eventHandler); err != nil { |
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.
Shouldn't we use ms.Close()
here ? Are we not missing the cancel()
call as well ?
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.
I am not so confident to call ms.Close()
in the Init
, as ms.Close()
is doing more things and may do even more in future, if it is not required, we should not couple them together.
So here I moved ms.ctx, ms.cancel = context.WithCancel(ctx)
to the end, actually, this ctx with cancel is used after Init.
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.
Actually, the caller may also call ms.Close()
when Init
returns error. At present, there is no problem.
But here I think we should avoid adding one more coupling if we can, in future, if ms.Close()
is not suitable to be called when Init
returns error, we should correct the caller.
|
||
defer func() { | ||
if !succeeded { | ||
if err := podInformer.RemoveEventHandler(podHandler); err != nil { |
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.
Shouldn't we use ms.Close()
here ? Are we not missing the cancel()
call as well ?
same as line 148 comment earlier. Curious to know if my understanding is incorrect.
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.
or do we need to combine both the defer calls for podhandler and eventhandler and then call ms.Close()
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.
See #7999 (comment)
Signed-off-by: Lyndon-Li <[email protected]>
b0bfd03
to
faa704d
Compare
@shubham-pampattiwar @sseago @blackpiglet I changed the code slightly per discussion #7999 (comment), please have another look. Thanks. |
Fix #7932. Data mover micro-service watcher according to design #7576