Skip to content
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

Deprecate Threadpool Configuration Properties #159

Open
kkoz opened this issue Feb 27, 2023 · 0 comments
Open

Deprecate Threadpool Configuration Properties #159

kkoz opened this issue Feb 27, 2023 · 0 comments

Comments

@kkoz
Copy link
Contributor

kkoz commented Feb 27, 2023

Relates to #154 #155.
Currently, the server maintains a threadpool for tasks classified as USER or BACKGROUND. This thread pool is implied in the docs to have a maximum size configurable by omero.threads.max_threads and a minimum pool size configurable by omero.threads.min_threads. Also, a thread timeout is supposed to be configurable with omero.threads.idle_timeout. However, beacuse the Java class ThreadPoolExecutor is used here with an ubounded LinkedBlockingQueue, this is largely not how these configs work. To paraphrase the java documentation, with a ThreadPoolExecutor you have 3 options for managing threads:

  1. A finite thread pool limited only by the core thread pool size and an infinite queue (this is what we currently have)
  2. An infinite thread pool and effectively no queue at all (this is how the SYSTEM level thread pool works)
  3. A finite thread pool and finite queue. In this case as new tasks come in threads are created until the core thread pool size has been reached. Then new tasks are queued until the queue is full. Finally if the queue is full new tasks will cause new threads to be created up to the maximum pool size. After that additional tasks are rejected.

We probably want to stick with option 1, but if so we should change the configuration properties and ThreadPool constructor signature to more accurately represent what's happening. We should have a new constructor which only takes 1 pool size argument. Instead of the max_threads and min_threads configs and constructor args, there should be a single value called core_threads. This is used to set the core thread pool size in the ThreadPool. By default, core threads do not time out, but this can be changed (see this). We can decide whether we want to allow the configuration of timeouts or not, in which case we may need to also change the omero.threads.idle_timeout property and constructor argument.

In summary:

  • There should be a new configuration property omero.threads.core_threads which sets the core thread pool size for the USER and BACKGROUND thread pool.
  • We should create a ThreadPool constructor which takes only this one value instead of a min and max value.
  • We should make a decision about whether to always enable timeout, always disable it, or make it configurable.
  • We should deprecate omero.threads.max_threads and omero.threads.min_threads and the ThreadPool constructor that uses them.
  • We should eventually remove the old constructor and either call the new one with core_threads if it's set and min_threads if it's not.
  • We should eventually remove omero.threads.max_threads and omero.threads.min_threads completely and perhaps throw an exception on server startup if they are set explaining that core_threads needs to be set instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant