Use Factory for informers #412
Conversation
| go func() { | ||
| <-ei.ctx.Done() | ||
| ei.stopped = true | ||
| close(stopCh) | ||
| DefaultFactoryStore.Stop(ei.FactoryIndex) | ||
| }() | ||
|
|
There was a problem hiding this comment.
This go routine (also ctx field) seems redundant after changes in Stop().
There was a problem hiding this comment.
I thought that this is still required 🙈 Could you please point me how to rearrange the code properly?
There was a problem hiding this comment.
I think we can just delete this go routine.
| @@ -299,7 +288,6 @@ func (m *monitor) Start(parentCtx context.Context) { | |||
|
|
|||
| if m.NamespaceInformer != nil { | |||
| m.NamespaceInformer.WithContext(m.ctx) | |||
| m.NamespaceInformer.WithSyncPeriod(m.informerSyncTime) | |||
There was a problem hiding this comment.
NamespaceInformer is not using factory. Is it OK to delete this setter and use 0 as syncTime in underlying wait.PollImmediateUntil?
There was a problem hiding this comment.
Good question. I used WithSyncPeriod previously to tune this for tests to reduce the startup time for tests, Since we start this informer only once, tuning the sync period doesn't win us much time and look like a complication without benefits.
There was a problem hiding this comment.
It seems we can use DefaultSyncPeriod from factory in namespace_informer, see this commit: 33f3aa9
WDYT?
There was a problem hiding this comment.
Not necessarily, but ok. We can do it.
Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
nabokihms
force-pushed
the
factory-test
branch
from
781804f
to
ab2f765
Compare
Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
Overview
Reuse watch and cache for informers
What this PR does / why we need it
The most resources shell-operator consume to encode and decode watch events.
Special notes for your reviewer
Does this PR introduce a user-facing change?