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.

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

Revisit SharedInformer support and do Improvements #1894

Closed
yybear opened this issue Dec 6, 2019 · 4 comments · Fixed by #1951
Closed

Revisit SharedInformer support and do Improvements #1894

yybear opened this issue Dec 6, 2019 · 4 comments · Fixed by #1951
Assignees

Comments

@yybear
Copy link
Contributor

yybear commented Dec 6, 2019

with 4.6.1

public void determineNextResync(ZonedDateTime now) { this.nextResync = now.plusSeconds(this.resyncPeriod); }

in DefaultShareIndexInformer.java

ProcessorListener<T> listener = new ProcessorListener(handler, determineResyncPeriod(resyncCheckPeriodMillis, this.resyncCheckPeriodMillis));

so using plusSeconds to get nextResync is ok?

@rohanKanojia
Copy link
Member

@yybear : nice catch. It seems like a bug to me. Could you please create a quick PR to fix this issue?

@yybear
Copy link
Contributor Author

yybear commented Dec 8, 2019

@rohanKanojia : another question, in golang reflector.go
// period controls timing between one watch ending and // the beginning of the next one. period time.Duration
func NewNamedReflector(name string, lw ListerWatcher, expectedType interface{}, store Store, resyncPeriod time.Duration) *Reflector { r := &Reflector{ name: name, listerWatcher: lw, store: store, expectedType: reflect.TypeOf(expectedType), period: time.Second, resyncPeriod: resyncPeriod, clock: &clock.RealClock{}, } return r }
so in reflector.go,period is different from resyncPeriod,is just one second.
but in fabric8 if FullResyncPeriod >0 , period and resyncPeriod are all equal FullResyncPeriod.
otherwise period is set to 5 seconds by default.

this is different from reflector.go, is this ok?

@rohanKanojia
Copy link
Member

@yybear : I'm not sure since it might be possible that client-go's codebase had undergone changes after we ported this from them. I think I have to take a look again to align informer support with latest client-go releases.

Do you mind if I pick this issue up instead? I think it might be more involved.

@yybear
Copy link
Contributor Author

yybear commented Dec 9, 2019

@rohanKanojia Yes, sure.

@rohanKanojia rohanKanojia self-assigned this Dec 18, 2019
@rohanKanojia rohanKanojia changed the title ProcessorListener determineNextResync should use millisencond not seconds? Revisit SharedInformer support and do Improvements Jan 8, 2020
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jan 14, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
    This PR contains some improvements/changes that happened since last port of
    this feature from official go client + official java client
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jan 14, 2020
    This PR contains some improvements/changes that happened since last port of
    this feature from official go client + official java client
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jan 17, 2020
    This PR contains some improvements/changes that happened since last port of
    this feature from official go client + official java client
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jan 20, 2020
    This PR contains some improvements/changes that happened since last port of
    this feature from official go client + official java client
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants