add a method to stop the mixer Store explicitly#3468
Conversation
Previously it accepted context.Context for the validity of the store, but that does not fit well with the mixer's calling structure. Also context's document says a struct shouldn't store the context as a field. Considering the situation, I think io.Closer would be better.
|
/assign @geeknoid |
mixer/pkg/runtime2/runtime.go
Outdated
| c.shutdown <- struct{}{} | ||
| c.shutdown = nil | ||
| if err := c.store.Close(); err != nil { | ||
| log.Errorf("Failed on close: %v", err) |
There was a problem hiding this comment.
Can you improve the error message (i.e. "store.Close failed during runtime shutdown: %v")
mixer/pkg/runtime2/runtime.go
Outdated
| if c.shutdown != nil { | ||
| c.shutdown <- struct{}{} | ||
| c.shutdown = nil | ||
| if err := c.store.Close(); err != nil { |
There was a problem hiding this comment.
Should the close be before c.waitQuiesceListening, or after?
The background go-routine will be listening to both the c.shutdown channel, as well as the channel returned by the store. When the store is closed here, what will happen to the channel that is returned by the store? (Or
There was a problem hiding this comment.
Ooh, nice catch! The watch channel shall be closed on store's Close(), that means the ordering is incorrect. watchChanges might not finish at all. Changed the order, waitQuiesceListening first, and then store.Close. Thank you!
|
@ozevren is this good to go now ? if so please / l g t m |
mixer/pkg/config/crd/store.go
Outdated
| var _ store.Backend = &Store{} | ||
| var _ probe.SupportsProbe = &Store{} | ||
|
|
||
| // Close implements io.Closer |
There was a problem hiding this comment.
Do we have to implement io.Closer? I don't think we're using this interface to reference the Store.
It looks to me, if we simply have a Close() method that returns nothing, we can simplify some of the client code paths.
There was a problem hiding this comment.
I personally think it's not a good idea to use the same name Close but does not implement io.Closer, I feel like it's one of very basic interface of golang.
So maybe it's better to rename it rather than simply changing return type -- replaced Stop. Let me know if you have other ideas.
mixer/pkg/config/crd/store.go
Outdated
| } | ||
|
|
||
| // checkAndCreateCaches checks the presence of custom resource definitions through the discovery API, | ||
| // and then create caches through lwBUilder which is in kinds. It retries within the timeout duration. |
There was a problem hiding this comment.
Update comments? There is no timeout anymore.
|
/test e2e-simple |
|
PTAL; also updated the PR title / comment. Note that the failures seem to be unrelated side effects of #3529, nothing by this PR. |
|
/test e2e-simple We renamed e2e-smoke to e2e-bookInfo and added e2e-simple |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ozevren The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Previously it accepted context.Context for the validity of the
store, but that does not fit well with the mixer's calling
structure. Also context's document says a struct shouldn't store
the context as a field.
This PR adds
Stopmethod to do this.