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

Should we remove the listener thread pool? #53049

Closed
6 tasks done
jasontedor opened this issue Mar 3, 2020 · 5 comments · Fixed by #53314
Closed
6 tasks done

Should we remove the listener thread pool? #53049

jasontedor opened this issue Mar 3, 2020 · 5 comments · Fixed by #53314
Labels
>breaking :Core/Infra/Core Core issues without another label Meta

Comments

@jasontedor
Copy link
Member

jasontedor commented Mar 3, 2020

Previously this thread pool was primarily used in the transport client to ensure that listeners are not called back on network threads.

We also adopted the use of this thread pool for global checkpoint listeners, which are callbacks that are executed at the shard level when the global checkpoint for the shard increases above a per-listener defined value.

With the removal of the transport client, this leaves global checkpoint listeners as the only use of the listener thread pool.

I would love to remove another thread pool from Elasticsearch.

Should we remove the listener thread pool from Elasticsearch? This would be a breaking change for any user that has manually configured the size of this thread pool. That would be unexpected, tuning this thread pool should not be necessary:

  • its only use in the server is for global checkpoint listeners
  • global checkpoint listeners are only registered when CCR is in use
  • its size should not be a limiting factor in any functionality on CCR since typically there would only be one listener outstanding per shard, and these listeners timeout after 30s

Therefore, while this would be a breaking change, we expect the impact on the user base to be extremely minimal.

@jasontedor jasontedor added >breaking :Core/Infra/Core Core issues without another label team-discuss labels Mar 3, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

@jasontedor
Copy link
Member Author

I missed an additional use of this thread pool which is that refresh listeners are called back on this thread pool.

We discussed this during the distributed team sync and agreed that we should aim for reducing the number of thread pools in Elasticsearch and settled on the following plan:

  • use the CCR thread pool for global checkpoint listeners that are queued from CCR
  • use the calling thread for refresh listeners, as those callbacks are lightweight

@jaymode
Copy link
Member

jaymode commented Mar 4, 2020

Should we remove the listener thread pool from Elasticsearch?

+1

Since this issue is still labeled team discuss and it was discussed during the distributed sync, is there something specific that needs further discussion?

@jasontedor
Copy link
Member Author

Since this issue is still labeled team discuss and it was discussed during the distributed sync, is there something specific that needs further discussion?

I've removed the label. 🙂

@jasontedor
Copy link
Member Author

@original-brownbear Thanks for being my review buddy on this one, and your helpful comments on the PRs!

jrodewig added a commit that referenced this issue Sep 22, 2021
Changes:
* Removes docs for the `listener` thread pool
* Adds an 8.0 breaking change for the thread pool removal

Relates to #53314 and #53049
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Core/Infra/Core Core issues without another label Meta
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants