-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
overhauling all threading concerns related to informers #3108
Conversation
Can one of the admins verify this patch? |
...netes-client/src/main/java/io/fabric8/kubernetes/client/informers/cache/SharedProcessor.java
Outdated
Show resolved
Hide resolved
kubernetes-client/src/test/java/io/fabric8/kubernetes/client/informers/SharedSchedulerTest.java
Outdated
Show resolved
Hide resolved
kubernetes-client/src/test/java/io/fabric8/kubernetes/client/informers/SharedSchedulerTest.java
Outdated
Show resolved
Hide resolved
kubernetes-client/src/main/java/io/fabric8/kubernetes/client/informers/SharedScheduler.java
Outdated
Show resolved
Hide resolved
...rnetes-client/src/main/java/io/fabric8/kubernetes/client/informers/ResourceEventHandler.java
Outdated
Show resolved
Hide resolved
using a shared resync scheduler allowing handlers to not have dedicated threads making run blocking
…nformers/cache/SharedProcessor.java Co-authored-by: Rohan Kumar <[email protected]>
…nformers/SharedSchedulerTest.java Co-authored-by: Rohan Kumar <[email protected]>
If the approaches here seem good, then logic in dsl.internal would be good to review for consolidating threading concerns. |
also adding a method to get the api class from the informer
Relates to: #2010 |
158a4e0
to
0bea58e
Compare
I've expanded the scope to include the the AbstractWatchManager thread as well - it will now use shared resources, rather than a dedicated thread. The only dedicated threads now related to informers will be held by okhttp. All of the resync related changes are potentially moot based upon #3159 |
kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/Utils.java
Show resolved
Hide resolved
moved some logic into util, creating a common scheduler and thread pool
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.
LGTM, thx!
SonarCloud Quality Gate failed. |
Description
Fix for #3072 and #3143, which includes:
fully removing the controller thread and making run blocking - this could be viewed as an important behavioral change. This requires not using the forkJoin pool in the informer factory because the async run calls will be blocking. However this allows anyone directly calling run to know immediately if the informer has been established without using a listener. An alternative is to add a separate runBlocking method and go back to passing in the executorservice into the DefaultSharedIndexInformer. Note the go implementation has a blocking run method - https://github.com/kubernetes/client-go/blob/f6ce18ae578c8cca64d14ab9687824d9e1305a67/tools/cache/controller.go#L127 https://github.com/kubernetes/client-go/blob/f6ce18ae578c8cca64d14ab9687824d9e1305a67/tools/cache/shared_informer.go#L157
made all thread pools daemon - by using a utility method
to create the daemon thread factory
inverting the paradigm for processorlistener execution
merging the controller class with defaultsharedindexinformer, since controller only had a minimal amount of responsibilities
since resync is in-memory only there is now only 1 shared scheduler for all informers, which is also reused by abstractwatchmanager
event handlers are processed in a thread off of main informer threadpool - but retain serial execution.
to remove the code smell of the run exception handling, SharedInformerEventListener should provide the informer #3143 was addressed by moving concerns about the informer listerners to the factory.
Type of change
test, version modification, documentation, etc.)
Checklist