che #14530 Changing 'che.workspace.pool.type' from 'fixed' to 'cached' by default

[ibuziuk]

What does this PR do?

Changing 'che.workspace.pool.type' from 'fixed' to 'cached' by default. This change will use CachedThreadPool:

Creates a thread pool that creates new threads as needed, but will reuse previously constructed threads when they are available. These pools will typically improve the performance of programs that execute many short-lived asynchronous tasks. Calls to execute will reuse previously constructed threads if available. If no existing thread is available, a new thread will be created and added to the pool. Threads that have not been used for sixty seconds are terminated and removed from the cache. Thus, a pool that remains idle for long enough will not consume any resources. Note that pools with similar properties but different details (for example, timeout parameters) may be created using ThreadPoolExecutor constructors

Originally implemented for 5.1.0 in #3701 the value has not been changed ever after. I suspect the default was applied mainly for local development and was overridden for codenvy.io / on-prem installations

What issues does this PR fix or reference?

#14614
Should also fix #14530

Docs PR

I'm not sure if the doc PR is needed since the configuration of the workspace pool type is suppose to be an internal detail. It might be interesting to document the recommended setup as part of Scalability, Performances and Working with high latency Roadmap item

[skabashnyuk]

Are there any facts that confirm that this will fix an issue #14530? Do you have any estimates on how it will affect thread/memory usage? Should we wait for #14601 and #14600 before merging this to get a better understanding of the problem and solution that you are going to provide?

[che-bot]

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

• build details
• "che-server" docker image: maxura/che-server:14631

[ibuziuk]

Should we wait for #14601 and #14600 before merging this to get a better understanding of the problem and solution that you are going to provide?

@skabashnyuk +1 for having monitoring first. In the meantime, I will apply this config to prod-preview

Do you have any estimates on how it will affect thread/memory usage

No, but according to docs the should be no issues with threads / ram on the distance:

Threads that have not been used for sixty seconds are terminated and removed from the cache. Thus, a pool that remains idle for long enough will not consume any resources.

[che-bot]

E2E tests of Eclipse Che Multiuser on OCP has failed:

• Build details
• Test report
• docker image: eclipseche/che-server:14631
  https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
  Use command "ci-test" to rerun the test.

[sleshchenko]

Previously, we had cached thread pool for JSON RPC messages and if I'm not mistaken it caused OutOfMemory crashing...
If we set cached without any limit - I assume it can lead to OutOfMemory crashing as well.
With JSON RPC it was easier to reproduce, but still...

Maybe it makes sense to consider this value to be configured along with the memory that Che container gets.
Like in che.properties it's fixed and every deployment provides his own values according to provided container memory.
To be more safe we could instead of reusing default java cached pool, introduce our own cached pool with a maximum size. We could estimate some maximum size according to container memory.

I'm not a big expert in this area, and it's just thoughts. In reality, my comments may be not so critical or not relevant at all.

[ibuziuk]

yeah, this sounds relevant - having no ability of spesifying maximum size is potentially risky. but again, according to the docs:

Threads that have not been used for sixty seconds are terminated and removed from the cache. Thus, a pool that remains idle for long enough will not consume any resources

So, it looks like technically we should not care about maximum size (if we should than there is an issue on the other level e.g. RAM misconfig etc)

[ibuziuk]

after discussion with @skabashnyuk and @skryzhny it looks like the situation with cached thread pool is not obvious:

• codenvy.io is using fixed and does not seem to have any issues with asyncStop / asyncStart
• using the cached could potentially be risky due to no upper limit of the thread number, even though docs are saying that Threads that have not been used for sixty seconds are terminated and removed from the cache. Thus, a pool that remains idle for long enough will not consume any resources. in reality the situation might be different and potentially result in OOM

[ibuziuk]

Closing for further investigation