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

microprofile-health-35 Provide different types of health check #142

Merged
merged 5 commits into from
Apr 26, 2019

Conversation

antoinesd
Copy link
Contributor

First version of the pull request for #35.
HealthCheckResponse description will probably need to be change to take Readiness into account.

@antoinesd antoinesd added the DO NOT MERGE PR in wip state label Sep 12, 2018
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@derekm derekm left a comment

Choose a reason for hiding this comment

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

Existing @Health checks cannot be assumed to be valid @Liveness checks.

spec/src/main/asciidoc/overview.adoc Outdated Show resolved Hide resolved
@rdebusscher
Copy link

Mention within spec this phrase (from initial request)

The readiness probe will return FAIL when the application is initializing itself.

This is important when

  • The developer didn't define any readiness check himself and just want the service to be identified as ready as soon it is fully deployed
  • In case the startup is in 2 phases, startup server and then deployment of the app) the readiness endpoint doesn't signal UP in the brief moment between server is started and before the app is deployed and readiness checks are picked up.

spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/overview.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/overview.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/overview.adoc Outdated Show resolved Hide resolved
@mkouba
Copy link

mkouba commented Feb 14, 2019

Well, I still find the @Health qualifier useless. I think that for /health endpoint all the health check producedures should be invoked, i.e. the impl should use @Inject @Any Instance<HealthCheck> to obtain all procedures. For /health/live the impl should use @Inject @Liveness Instance<HealthCheck> and for /health/ready the impl should use @Inject @Readyness Instance<HealthCheck>.

I would also recommend to add a new annotation to specify custom health checks groups, something like @HealthCheckGroup("checksA") (but find a better name ;-). @Named is probably not appropriate as it has side effects (named beans are accessible in EL etc.). This way it would be possible to select only specific checks for /health/{groupName} endpoints, using @Inject @Any Instance<HealthCheck> and instance.select(HealthCheckGroup.Literal.of("checksA")).

@xstefank
Copy link
Member

@mkouba I really like your idea in the second paragraph but this PR probably isn't the right place to track it. Can you create an issue for it please?

@mkouba
Copy link

mkouba commented Feb 14, 2019

@xstefank I've added a comment to #81...

@antoinesd antoinesd force-pushed the microprofile-health-35 branch from 2609cf8 to 3bd1c10 Compare March 29, 2019 14:47
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/overview.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/overview.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/protocol-wireformat.adoc Outdated Show resolved Hide resolved
@Ladicek
Copy link

Ladicek commented Apr 19, 2019

I'd avoid using terms like "MP-HC". This is AsciiDoc -- if "MicroProfile Health Check" is too long to write all the time, use a variable :-)

@antoinesd
Copy link
Contributor Author

I agree @Ladicek. But for clarity I suggest that we manage that in another ticket / PR

Signed-off-by: Antoine Sabot-Durand <antoine@sabot-durand.net>
@antoinesd antoinesd force-pushed the microprofile-health-35 branch from 85d09fe to 8a18483 Compare April 23, 2019 09:53
Correct old typo

Signed-off-by: Antoine Sabot-Durand <antoine@sabot-durand.net>
Copy link
Member

@xstefank xstefank left a comment

Choose a reason for hiding this comment

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

one small mistake, can be fixed in another PR. Thanks!

spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
Typo correction

Signed-off-by: Antoine Sabot-Durand <antoine@sabot-durand.net>
@antoinesd antoinesd removed the DO NOT MERGE PR in wip state label Apr 23, 2019
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/java-api.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/overview.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/overview.adoc Outdated Show resolved Hide resolved
Add more review

Signed-off-by: Antoine Sabot-Durand <antoine@sabot-durand.net>
@antoinesd antoinesd requested a review from Emily-Jiang April 24, 2019 08:38
@Emily-Jiang
Copy link
Member

One more comment: please increase the package-info version from 1.0 to 1.1 since new APIs were added.

Copy link
Member

@Emily-Jiang Emily-Jiang left a comment

Choose a reason for hiding this comment

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

Please update package-info.java. Sorry, I missed this in my last review.

Update OSGI API version

Signed-off-by: Antoine Sabot-Durand <antoine@sabot-durand.net>
@antoinesd
Copy link
Contributor Author

@Emily-Jiang are you sure it should be 1.1 and not 2.0 to follow spec version?

@antoinesd
Copy link
Contributor Author

I merge with required version modification.

@antoinesd antoinesd merged commit 078b89a into microprofile:master Apr 26, 2019
@antoinesd antoinesd deleted the microprofile-health-35 branch April 26, 2019 09:29
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.

8 participants