CQL ThreadPoolExecutor has a fixed size

[cdegroc]

Hi all,

CQLStoreManager/CQLKeyColumnValueStore instantiate an ExecutorService that is used to convert CQL ResultSets to a JanusGraph EntryList, among other things.

That ExecutorService is declared here (master branch) as follows

this.executorService = new ThreadPoolExecutor(
    10,                             // corePoolSize
    100,                            // maximumPoolSize
    1,                              // keepAliveTime
    TimeUnit.MINUTES,               // unit
    new LinkedBlockingQueue<>(),    // workQueue
    new ThreadFactoryBuilder()      // threadFactory
            .setDaemon(true)
            .setNameFormat("CQLStoreManager[%02d]")
            .build());

Given that corePoolSize=10 and maximumPoolSize=100, I was expecting that ThreadPool to dynamically grow/shrink as needed. However instrumentation (using an InstrumentedExecutorService) showed that the ThreadPool was always using 10 threads.

This is explained in the ThreadPoolExecutor javadoc:

Using an unbounded queue (for example a LinkedBlockingQueue without a predefined capacity) will cause new tasks to wait in the queue when all corePoolSize threads are busy. Thus, no more than corePoolSize threads will ever be created. (And the value of the maximumPoolSize therefore doesn't have any effect.)

I think we could fix this in two ways:

1. Make the code clearer by switching to a newFixedThreadPool(10, new ThreadFactoryBuilder()...).
2. Make the ThreadPool really dynamic by using a custom ThreadPoolExecutor implementation (see this article for more details and this ElasticSearch implementation for reference).

Happy to get feedback and contribute a PR if there's interest.
I'm also curious to know if this could be a bottleneck in JanusGraph (and also, if not, why).

[porunov]

Thank you for starting this topic @cdegroc !
I would prefer a second option. I.e. I would imagine the pool to act something like cached thread pool which respects maximumPoolSize and corePoolSize. I think it would be great to see a contribution in this area.
I also think that corePoolSize and maximumPoolSize should be configurable via properties.

[li-boxuan]

Actually I don't even understand why we need a thread pool here. 😅 Can someone explain? Thanks!

[li-boxuan]

Thanks for the detailed explanation! I guess my confusion actually comes from: why do we need to use vavr Future at the first place? Maybe I am missing something, but why not just

Future future = this.session.executeAsync(this.getSlice.bind().....
return fromResultSet(future.get(), this.getter);

[cdegroc]

It was there from the start. Your guess is as good as mine :)
Could another JanusGraph dev have more insights?
Otherwise I can test without it and observe the effects on threads states and performance metrics but that will require more time.

[li-boxuan]

Ping the authors: @pkendall64 @samantmaharaj

[li-boxuan]

I am just guessing: maybe the author(s) wanted to use the thread pool as a bulkhead to limit the number of concurrent backend requests?

[li-boxuan]

FYI @porunov created a PR: https://github.com/JanusGraph/janusgraph/pull/2700/files to make thread pool size configurable

[porunov]

@li-boxuan , @cdegroc I am thinking to change the default configuration for CQL ExecutorService to be disabled by default after #2741 . I'm not changing the default configuration in that PR to not debate about the default configurations in the PR but if that PR is merged I was thinking to propose to change the default settings to be disabled and add additional information to the documentation so that users would know that this option is available historically but isn't recommended. What do you think about it?

Performance tests conducted:
#2741 (comment)
#2741 (comment)
#2741 (comment)
#2741 (comment)

[li-boxuan]

I don't have a strong opinion but I am a bit inclined to defer this to the next (major) release. It might be the case that with the ExecutorService disabled, heavy workloads will start to fail in production environments, and users will have to tune the max-requests-per-connection. This is an example where the workload is so heavy that queries start to fail. This is less likely to happen when the executor service is used because it limits the number of concurrent requests.

That being said, I am fine with releasing it in this release if we emphasize that existing users should be careful and keep ExecutorService enabled before they thoroughly test and tune the configs.

[porunov]

Good thoughts about tuning. Maybe we should leave executor service enabled for this release but emphasize that's its recommended to disable it in production.

[porunov]

CQL ThreadPoolExecutor was extended to support multiple different implementations (as well as custom implementations). Moreover, CQL storage backend doesn't use ThreadPoolExecutor to manage IO operations anymore. Instead, it reuses DataStax Async approach and uses ThreadPoolExecutor for deserialization work only. Thus, fixed thread pool of cores * 2 is now preferable size in most cases.
See PR #3760 for more information. Closing this discussion as resolved, but feel free to reopen in case we missed anything.