PAYARA-3829 MicroProfile Healthcheck 2.1 implementation #4254
Conversation
There was a problem hiding this comment.
I only have a rough understanding of MP Health so don't take my comments as gospel. It might just be me being confused or picky. What I would like to see overall is a nicer more consistent handling of the 3 types of checks where the type specific part is extracted to e.g. the enum so that the rest of the code can be generic to avoid repeatedly making switches or if-else blocks doing slightly different things for the 3 types we have. Some of my suggestions might help to understand what I mean but I don't think just following all of them would give something fully consistent either. They are more thoughts I had when looking at the particular situations.
...ofile/healthcheck/src/main/java/fish/payara/microprofile/healthcheck/HealthCheckService.java
Outdated
Show resolved
Hide resolved
...ofile/healthcheck/src/main/java/fish/payara/microprofile/healthcheck/HealthCheckService.java
Outdated
Show resolved
Hide resolved
...ofile/healthcheck/src/main/java/fish/payara/microprofile/healthcheck/HealthCheckService.java
Outdated
Show resolved
Hide resolved
...ofile/healthcheck/src/main/java/fish/payara/microprofile/healthcheck/HealthCheckService.java
Outdated
Show resolved
Hide resolved
...oprofile/healthcheck/src/main/java/fish/payara/microprofile/healthcheck/HealthCheckType.java
Show resolved
Hide resolved
...althcheck/src/main/java/fish/payara/microprofile/healthcheck/servlet/HealthCheckServlet.java
Outdated
Show resolved
Hide resolved
...ofile/healthcheck/src/main/java/fish/payara/microprofile/healthcheck/HealthCheckService.java
Show resolved
Hide resolved
…named to MP_HEALTH_BACKWARD_COMPATIBILITY_ENABLED
|
jenkins test please |
There was a problem hiding this comment.
Looks good, but one quibble.
Tested using my MicroProfileOnMicro app deployed against DAS (after updating health example to 2.1)
| @PostConstruct | ||
| public void postConstruct() { | ||
| if (events == null) { | ||
| events = Globals.getDefaultBaseServiceLocator().getService(Events.class); | ||
| } | ||
| events.register(this); | ||
| this.backwardCompEnabled = ConfigProvider.getConfig() |
There was a problem hiding this comment.
Should this also be checked on each invocation?
As it is, it's not dynamic which slightly defeats one of the purposes of MP Config.
There was a problem hiding this comment.
I purposely added this snippet to PostConstruct method instead of each invocation for very minor optimization. Do you think, end-user will need to change MP_HEALTH_BACKWARD_COMPATIBILITY_ENABLED property value dynamically?
There was a problem hiding this comment.
In 95% of cases, probably not.
But it's the fact that by being only checked in the postconstruct it essentially means that a restart is required if you want to change this. We can't however actually indicate that, because since it's MP config the option can be defined outside of Payara, so we have no way to check if it has changed except by, well, running this exact check :P
It's probably fine as is, we'd just need to make sure to explicitly document this case.
Description
MicroProfile Healthcheck 2.0 and 2.1 implementation:
outcomeandstatereplaced bystatus[Breaking Change]@Livenessand@Readinesson/health/readyand/health/liveendpointsoutcomeandstatenot replaced bystatusin JSON payload) of/healthendpoint set the value ofMP_HEALTH_BACKWARD_COMPATIBILITY_ENABLEDMP config property to true./healthis deprecated and return combines Health checks data of@Liveness,@Readinessand@Health.Important Info
Dependant PRs
payara/MicroProfile-TCK-Runners#59
Testing
New tests
Microprofile HealthCheck TCK v2.1
Test suites executed
mp.health.enable-backward-compatibility=true