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.

Sign up for GitHub

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

Jump to bottom

Provide a way to run HealthIndicators concurrently #17617

Closed
wants to merge 2 commits into from
Closed

Provide a way to run HealthIndicators concurrently #17617

wants to merge 2 commits into from

Conversation

nosan
Copy link
Contributor

@nosan nosan commented Jul 24, 2019

see gh-2652

Please let me know if I need something to document.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 24, 2019
snicoll
snicoll reviewed Jul 24, 2019
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @nosan, that's very interesting.

I think we need to look at what we have on the reactive side of things and harmonize or update the PR in such a way the config is handled consistently. I've added a few suggestions

...ain/java/org/springframework/boot/actuate/autoconfigure/health/HealthEndpointProperties.java Outdated
/**
* Whether health indicators should be called concurrently.
*/
private boolean enabled;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is strictly related to the imperative approach as the reactive approach already benefits from that feature transparently. Given that indicators are already invoked in parallel I wonder if we should offer an option at all.

...ain/java/org/springframework/boot/actuate/autoconfigure/health/HealthEndpointProperties.java Outdated
/**
* Timeout to wait for the result from health indicators.
*/
private Duration timeout;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reactive approach has a strategy where both a timeout and a fallback can be provided. I think we should harmonize that here too.

...rc/main/java/org/springframework/boot/actuate/health/CompositeConcurrentHealthIndicator.java Outdated
* @author Dmytro Nosan
* @since 2.2.0
*/
public class CompositeConcurrentHealthIndicator implements HealthIndicator {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't create a separate class for this. Perhaps there is a way to extract the behavior in a Function or something like that and just configure it externally when required?

Either way, this feature should be backed back in CompositeHealthIndicator. We currently have a PRs on supporting health groups that relies on a single composite implementation.

@nosan
Copy link
Contributor Author

nosan commented Jul 24, 2019

Thank you very much @snicoll

PR has been updated.

I believe it is what you are expecting.

@nosan
Copy link
Contributor Author

nosan commented Jul 24, 2019 •
edited
Loading

From another point of view, maybe it makes sense to introduce HealthIndicatorStrategy interface.
For now, it could be two different implementations:

  • DefaultHealthIndicatorStrategy - returns health indications from all HealthIndicators sequentially
  • ConcurrentHealthIndicatorStrategy - returns health indications from all HealthIndicators concurrently

Update CompositeHealthIndicator to use HealthIndicatorStrategy to collect health results and finally expose HealthIndicatorStrategy as a bean.

@nosan
Copy link
Contributor Author

nosan commented Jul 25, 2019 •
edited
Loading

@snicoll
There are 2 approaches:

  1. Update CompositeHealthIndicator with new methods parallel and sequential to configure a strategy that should be used. Is not possible to define your own strategy at all. There is no way to configure strategy via bean.
  2. Update CompositeHealthIndicator to use HealthIndicatorStrategy. Currently, I've added two strategies ConcurrentlyHealthIndicatorStrategy and DefaultHealthIndicatorStrategy. Easy to define your own strategy. Strategy can be registered as a bean.

Please let me know what you think.
Thanks in advance.

@snicoll
Copy link
Member

snicoll commented Aug 23, 2019

@nosan there was a major refactoring of the health support this week. Would you be interested to revisit your original proposal against master ?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Aug 23, 2019
@nosan
Copy link
Contributor Author

nosan commented Aug 23, 2019

@snicoll
sure, but I need some time to check this out.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 23, 2019
@nosan
Copy link
Contributor Author

nosan commented Aug 29, 2019

@snicoll
I'm closing this for now, as I need more time than I have expected.
Sorry for the inconvenience and thank you for your time.

@nosan nosan closed this Aug 29, 2019
@snicoll
Copy link
Member

snicoll commented Aug 29, 2019

No need to apologize @nosan and thank you for the feedback.

@snicoll snicoll removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Aug 29, 2019
@nosan nosan deleted the gh-2652 branch January 28, 2020 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snicoll snicoll snicoll left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nosan @snicoll @spring-projects-issues