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

Refactored code to allow the use of a custom ExecutorService #756

Merged
merged 1 commit into from
May 4, 2022

Conversation

dhoard
Copy link
Collaborator

@dhoard dhoard commented Jan 25, 2022

Refactored code to allow the use of a custom ExecutorService.

Code was refactored since part of the building logic was in HTTPServer.Builder and part of the logic was in a HTTPServer constructor, resulting in no good way to introduce the ExecutorService. With the refactor, all HTTPServer constructors now use the HTTPServer.Builder for a single point of construction.

Added TestHTTPServerBuilder to validate all of the possible builder configurations.

Signed-off-by: Doug Hoard doug.hoard@gmail.com

@dhoard
Copy link
Collaborator Author

dhoard commented Jan 25, 2022

@fstab please review.

@fstab
Copy link
Member

fstab commented May 2, 2022

Thanks a lot for the PR. There are a lot of changes in this one, and I feel it would be easier to split this into independent PRs that can be discussed separately.

In general, people should use the Builder to create the HTTPServer and not call the constructor directly. We have some public constructors that we need to keep for backwards compatibility, but new configuration (like SSL config) will be implemented in the Builder and not in the constructor.

So in order to make the ExecutorService configurable, it would be sufficient to add the withExecutorService() method to the Builder and set the executor service in Builder.build().

I know this PR is a couple of months old. Are you still available to continue this? It would be good if you could strip it down to a smaller PR that just adds the withExecutorService().

Signed-off-by: Doug Hoard <doug.hoard@gmail.com>
@dhoard
Copy link
Collaborator Author

dhoard commented May 3, 2022

@fstab code has been refactored. Please review.

@fstab fstab merged commit c61d2eb into prometheus:master May 4, 2022
@fstab
Copy link
Member

fstab commented May 4, 2022

Thanks a lot!

@dhoard dhoard deleted the issue_753 branch October 4, 2023 02:51
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

Successfully merging this pull request may close these issues.

None yet

2 participants