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

Optimization/Features/Suggestions around managed executors #3287

Closed
dennyac opened this issue May 9, 2020 · 2 comments
Closed

Optimization/Features/Suggestions around managed executors #3287

dennyac opened this issue May 9, 2020 · 2 comments
Labels
stale Stale issue or pull request which will be closed soon

Comments

@dennyac
Copy link
Contributor

dennyac commented May 9, 2020

Optimization/Features/Suggestions around managed executors

  1. The current default thread factory creates a new DefaultThreadFactory instance whenever a new thread is constructed.
  2. For threads we have a global counter. Would prefer a counter per executor service (Use a global pool and local thread number similar to DefaultThreadFactory). Can move threadFactory logic to its own class. Would've extended threadFactory but its a private static class.
  3. nameFormat seems very unintuitive. It's not clear to users that they have to specify one. Also its usage across classes vary. (ScheduledExecutorServiceBuilder doesn't have that requirement).
  4. People who aren't familiar with the ThreadPoolExecutor may run into issues like Environment.lifecycle().executorService(...) might create single-threaded pools #2553 when using the ExecutorServiceBuilder.

PR for point 1 - #3285

For points 2, 3 and 4, I've created a draft PR and mentioned changes below. Don't think anything here can be considered as breaking changes. Can update release notes/docs/tests if you think this is a reasonable approach - #3286

  • User passes a prefix and the threadFactory will add pool/thread number.
  • Whatever the user passes is what is used in the metric name. The methods getNameWithoutFormat in this Executor metrics shouldn't contain fmt string. #3142 will return a string appended to itself, so it may have to be reverted. @pkwarren - Any feedback/concerns?
  • Includes managed and instrumented executor service factory methods. ThreadPoolExecutor Javadocs urges users to use the Executors factory methods that preconfigure settings for the most common usage scenarios. Also the InstrumentedExecutorService has additional metrics like idle/duration timers which InstrumentedThreadFactory (used in ExecutorServiceBuilder) doesn't have
@dennyac
Copy link
Contributor Author

dennyac commented May 10, 2020

Event if a subset of these changes are acceptable, let me know and I'll make appropriate changes/modifications

@joschi joschi linked a pull request May 10, 2020 that will close this issue
@github-actions github-actions bot added the stale Stale issue or pull request which will be closed soon label Aug 11, 2020
@dropwizard dropwizard deleted a comment from github-actions bot Aug 11, 2020
@joschi joschi removed the stale Stale issue or pull request which will be closed soon label Aug 11, 2020
@github-actions github-actions bot added the stale Stale issue or pull request which will be closed soon label Nov 10, 2020
@dropwizard dropwizard deleted a comment from github-actions bot Nov 10, 2020
@joschi joschi removed the stale Stale issue or pull request which will be closed soon label Nov 10, 2020
@github-actions
Copy link

This issue is stale because it has been open 90 days with no activity. Remove the "stale" label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale Stale issue or pull request which will be closed soon label May 10, 2021
@github-actions github-actions bot closed this as completed Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale issue or pull request which will be closed soon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants