-
Notifications
You must be signed in to change notification settings - Fork 1
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
[DCJ-457] Stairway can be built with a provided ThreadPoolTaskExecutor #149
[DCJ-457] Stairway can be built with a provided ThreadPoolTaskExecutor #149
Conversation
This allows calling services to bring their own instrumented executor to serve as their Stairway's threadpool. If unspecified in StairwayBuilder, a DefaultThreadPoolTaskExecutor will be constructed and used. DefaultThreadPoolTaskExecutor is a public class: calling services may elect to instantiate it themselves (e.g. a Spring Boot service would register this as a Bean for automatic actuator instrumentation).
(Only marked as "ready for review" to see if that helps with checks not running… but it seems that GitHub is experiencing issues: https://www.githubstatus.com/incidents/9vwllhs2w1kj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me -- I like the approach of decoupling StairwayThreadPool
from the executor (instead of inheriting from it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! The reason I put in request changes is about some questions. All minor but probably worthy of calling out
stairway/src/main/java/bio/terra/stairway/DefaultThreadPoolTaskExecutor.java
Show resolved
Hide resolved
stairway/src/main/java/bio/terra/stairway/impl/StairwayImpl.java
Outdated
Show resolved
Hide resolved
If this value were to change in the future, the javadoc rendered view would reflect the change.
Mainly because my usage of Optional.ofNullable…orElse… constructs a new default executor every time, and throws it away if the builder has one set. This is discouraged. I decided to adhere with the existing convention for initializing instance variables from the StairwayBuilder object, which makes the constructor easier to follow. If it's updated in the future with Optional handling, that would be acceptable as long as it's updated completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've answered all the questions I had :). Looks great!
This matches TDR's current build file -- the useMavenLocal flag makes it a little easier to leverage locally published artifacts on demand.
|
Addresses
Partially addresses https://broadworkbench.atlassian.net/browse/DCJ-457
PRs will need to be merged in the following order:
Summary of changes
This PR allows calling services to bring their own instrumented executor to serve as their Stairway's threadpool.
If unspecified in
StairwayBuilder
, aDefaultThreadPoolTaskExecutor
will be constructed and used.DefaultThreadPoolTaskExecutor
is a public class: calling services may elect to instantiate it themselves (e.g. a Spring Boot service would register this as a Bean for automatic actuator instrumentation).Testing Strategy
Expanded unit tests.
Verified end-to-end behavior manually:
maxParallelFlights
specifiedstairwayExecutor
metrics were automatically flowing through TDR's Actuator endpointExample where TDR configured its
maxParallelFlights
to be 120, has one actively running flight, and one completed flight: