feat: use only one, configurable ExecutorService #546
Conversation
| @@ -146,7 +124,13 @@ private void executeBufferedEvents(String customResourceUid) { | |||
| latestCustomResource.get(), | |||
| retryInfo(customResourceUid)); | |||
| log.debug("Executing events for custom resource. Scope: {}", executionScope); | |||
| executor.execute(new ExecutionConsumer(executionScope, eventDispatcher, this)); | |||
| executor.execute(() -> { | |||
There was a problem hiding this comment.
Actually, I'm probably going to revert that change so that we can override toString to get more precise debugging information on the Runnable… 🥲
There was a problem hiding this comment.
What I meant eliminating the ExecutionConsumer is elegant here
There was a problem hiding this comment.
Yes, I understood that but like I said, I actually need to re-introduce some equivalent form of it :)
| executor.execute(new ExecutionConsumer(executionScope, eventDispatcher, this)); | ||
| executor.execute(() -> { | ||
| // change thread name for easier debugging | ||
| Thread.currentThread().setName("EventHandler-" + controllerName); |
There was a problem hiding this comment.
Is this needed? It looks a bit strange that we change the thread name on every execution.
There was a problem hiding this comment.
Well, that was previously done when initializing the thread pool by specifying the thread factory. We can't do that anymore so the only alternative I could think of at that point was to rename the thread when it executes.
|
LGTM |
This would allow easier diagnosis when the thread is rejected.
metacosm
force-pushed
the
fix-540
branch
2 times, most recently
from
97772a4 to
ef27cba
Compare
The goal is to make sure the thread pool is properly started and shut down at appropriate time, while getting it ready for instrumentation. This should also address the tests issue where the executor was shut down at the end of one test and never re-started at the beginning of the next one thus leading to tasks being rejected.
This is done by moving the close method to static and nulling the singleton so that it can be re-created on next Operator start if needed. This is needed for proper testing support.
Fixes #540
cc @lburgazzoli