Borrow WC variants of executor views to reduce thread count and increase reuse #4877
Conversation
| LinkedBlockingQueue<Runnable> workQueue = new LinkedBlockingQueue<>(); | ||
| NamedThreadFactory threadFactory = new NamedThreadFactory("Atlas Cassandra Async KVS", false); | ||
|
|
||
| return PTExecutors.newThreadPoolExecutor(0, maxPoolSize, 1, TimeUnit.MINUTES, workQueue, threadFactory); |
There was a problem hiding this comment.
This executor configuration is highly suspect. At a glance I believe it will only ever have a single thread unless we manage to fill the LinkedBlockingQueue with Integer.MAX_VALUE elements.
There was a problem hiding this comment.
We looked at metrics-platform and it turns out this executor is basically unused in practice...
…ase reuse Note that this includes an API+ABI break for fixed-size-executor factory methods which have been updated to return ExecutorService instead of ThreadPoolExecutor.
carterkozak
force-pushed
the
ckozak/shared_executors
branch
from
78c8ead to
8e42db5
Compare
carterkozak
changed the title
carterkozak
changed the base branch from
ckozak/atlas_fixed_executor_consolidation
to
develop
| LinkedBlockingQueue<Runnable> workQueue = new LinkedBlockingQueue<>(); | ||
| NamedThreadFactory threadFactory = new NamedThreadFactory("Atlas Cassandra Async KVS", false); | ||
|
|
||
| return PTExecutors.newThreadPoolExecutor(0, maxPoolSize, 1, TimeUnit.MINUTES, workQueue, threadFactory); |
There was a problem hiding this comment.
We looked at metrics-platform and it turns out this executor is basically unused in practice...
| - type: break | ||
| break: | ||
| description: |- | ||
| `PTExecutors.newFixedThreadPool` overloads return `ExecutorService` instead of the concrete `ThreadPoolExecutor` type. |
There was a problem hiding this comment.
note: will cause breaks in large internal product. Can confirm not used by the common AtlasDB supporting libraries.
There was a problem hiding this comment.
I'm planning to fix the internal product once this is available.
| * Borrowed from jboss-threads. http://www.apache.org/licenses/LICENSE-2.0 | ||
| * https://github.com/jbossas/jboss-threads/blob/master/src/main/java/org/jboss/threads/QueuedViewExecutor.java Changes | ||
| * have been contributed and merged, this may be replaced by the upstream ViewExecutor pending a release including | ||
| * https://github.com/jbossas/jboss-threads/pull/85. |
There was a problem hiding this comment.
This PR merged, though I wouldn't block on us using the alternative there. I checked for equivalence - it seems we have a bit more interrupt handling logic here.
There was a problem hiding this comment.
pull/86 hasn't merged quite yet, but it's much less substantial than the others. Folks on their end are busy.
| @@ -239,20 +232,7 @@ public static LocalPaxosServices createInstrumentedLocalServices( | |||
| // TODO (jkong): Make the limits configurable. | |||
| // Current use cases tend to have not more than 10 (<< 100) inflight tasks under normal circumstances. | |||
| private static ExecutorService createExecutor(MetricsManager metricsManager, String useCase, int corePoolSize) { | |||
There was a problem hiding this comment.
nit: this is private so we could probably remove the unused corepoolsize parameter.
There was a problem hiding this comment.
iirc it's passed through several methods and I wanted to keep the code churn to a minimum.
Goals (and why):
Fewer total threads.
Threads are reused effectively.
Implementation Description (bullets):
Testing (What was existing testing like? What have you done to improve it?):
The executor code is already used in wc which has been rolled out broadly.