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

PAYARA-2479 PAYARA-3118 MP health check checker #3125

Merged
merged 13 commits into from Oct 29, 2018

Conversation

Projects
None yet
6 participants
@Cousjava
Member

Cousjava commented Sep 6, 2018

No description provided.

@Cousjava Cousjava added this to the Payara 5.184 milestone Sep 6, 2018

@Cousjava Cousjava self-assigned this Sep 6, 2018

@Cousjava

This comment has been minimized.

Member

Cousjava commented Sep 6, 2018

Jenkins test please

@payara-ci

This comment has been minimized.

Contributor

payara-ci commented Sep 6, 2018

Quick build and test passed!

@@ -1,6 +1,6 @@
# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
#
# Copyright (c) [2017] Payara Foundation and/or its affiliates. All rights reserved.
# Copyright (c) [20172-2018] Payara Foundation and/or its affiliates. All rights reserved.

This comment has been minimized.

@MeroRai

MeroRai Sep 6, 2018

Contributor

Should be "2017"

@@ -70,6 +70,8 @@ healthcheck.configuration.stuckThreadsCheckerTabTitle=Stuck Threads
healthcheck.configuration.stuckThreadsCheckerTabTitleToolTip=Configures the Stuck Threads HealthCheck.
healthcheck.configuration.garbageCollectorCheckerTabTitle=Garbage Collector
healthcheck.configuration.garbageCollectorCheckerTabTitleToolTip=Configures the Garbage Collector HealthCheck.
healthcheck.configuration.mircroprofileHealthcheckCheckerTabTitle=Microprofile Healthcheck

This comment has been minimized.

@MeroRai

MeroRai Sep 6, 2018

Contributor

Isn't it spelled MicroProfile

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 10, 2018

Member

Yes, it is "MicroProfile"
Also, the property has a typo as well: mircro

<dependency>
<groupId>fish.payara.admingui</groupId>
<artifactId>console-common</artifactId>
<version>5.182-SNAPSHOT</version>

This comment has been minimized.

@MeroRai

MeroRai Sep 6, 2018

Contributor

Should be 5.184-SNAPSHOT

@@ -119,6 +121,11 @@ healthcheck.checker.configuration.hoggingThreads.retryCountLabelHelpText=Number
healthcheck.checker.configuration.garbageCollector.pageTitle=Garbage Collector
healthcheck.checker.configuration.garbageCollector.pageTitleHelpText=Configuration options for the Garbage Collector HealthCheck.
healthcheck.checker.configuration.mircroprofileHealthcheckChecker.pageTitle=Garbage Collector

This comment has been minimized.

@MeroRai

MeroRai Sep 6, 2018

Contributor

Wrong title

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 10, 2018

Member

mircro typo again

@@ -119,6 +121,11 @@ healthcheck.checker.configuration.hoggingThreads.retryCountLabelHelpText=Number
healthcheck.checker.configuration.garbageCollector.pageTitle=Garbage Collector
healthcheck.checker.configuration.garbageCollector.pageTitleHelpText=Configuration options for the Garbage Collector HealthCheck.
healthcheck.checker.configuration.mircroprofileHealthcheckChecker.pageTitle=Garbage Collector
healthcheck.checker.configuration.mircroprofileHealthcheckChecker.pageTitleHelpText=Configuration options for the Microprofile Healthcheck HealthCheck.</br>Checks the status of the Microprofile Healthcheck endpoint for all instances in the domain

This comment has been minimized.

@MeroRai

MeroRai Sep 6, 2018

Contributor

"Healthcheck HealthCheck" ?

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 10, 2018

Member

mircro again x2

@@ -70,6 +70,8 @@ healthcheck.configuration.stuckThreadsCheckerTabTitle=Stuck Threads
healthcheck.configuration.stuckThreadsCheckerTabTitleToolTip=Configures the Stuck Threads HealthCheck.
healthcheck.configuration.garbageCollectorCheckerTabTitle=Garbage Collector
healthcheck.configuration.garbageCollectorCheckerTabTitleToolTip=Configures the Garbage Collector HealthCheck.
healthcheck.configuration.mircroprofileHealthcheckCheckerTabTitle=Microprofile Healthcheck

This comment has been minimized.

@MeroRai

MeroRai Sep 6, 2018

Contributor

To avoid confusion with MicroProfile Healthcheck. I think the title should have "checker" at the end.

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 10, 2018

Member

mircro! (╯°□°)╯︵ ┻━┻

@MeroRai

This comment has been minimized.

Contributor

MeroRai commented Sep 6, 2018

In my opinion, it makes much more sense to have the Healthcheck checker page along with MicroProfile health check. So, that way it is much more obvious on what it does.

* @author jonathan coustick
* @since 5.184
*/
@Service(name = "healthcheck-mp-configure")

This comment has been minimized.

@MeroRai

MeroRai Sep 7, 2018

Contributor

To remain consistent with other asadmin commands, it should be named along the lines of set-healthcheck-mp-configuration or set-healthcheck-checker-configuration.

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 10, 2018

Member

set-healthcheck-checker-configuration

This comment has been minimized.

@Cousjava

Cousjava Sep 10, 2018

Member

This command is consistent with other healthcheck checker commands, i.e. healthcheck-stuckthreads-configure

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 11, 2018

Member

That doesn't mean that those are in line with the naming policy.
Those are also for the Payara Health check service, not MicroProfile Health check (as this is using).
Please rename it :)

This comment has been minimized.

@Cousjava

Cousjava Sep 11, 2018

Member

This is for the Payara health check service. While those other ones may not be in line, I feel at least all the healthcheck ones should be consistent and if they are changed, all changed at once.

@Cousjava

This comment has been minimized.

Member

Cousjava commented Sep 10, 2018

Jenkins test please

@payara-ci

This comment has been minimized.

Contributor

payara-ci commented Sep 10, 2018

Quick build and test passed!

@Cousjava Cousjava force-pushed the Cousjava:PAYARA-2479-health-check-checker branch from 7b648fa to 9dbe47a Sep 11, 2018

@Cousjava

This comment has been minimized.

Member

Cousjava commented Sep 11, 2018

Jenkins test please

@payara-ci

This comment has been minimized.

Contributor

payara-ci commented Sep 11, 2018

Quick build and test failed!

@Cousjava

This comment has been minimized.

Member

Cousjava commented Sep 13, 2018

Jenkins test please

@payara-ci

This comment has been minimized.

Contributor

payara-ci commented Sep 13, 2018

Quick build and test passed!

Changes made

@Pandrex247

Naming grumbles

*/
@Service(name = "healthcheck-mp")
@RunLevel(10)
public class MPChecker extends BaseHealthCheck<TimeoutHealthCheckExecutionOptions, MPCheckerConfiguration> implements PostConstruct {

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 19, 2018

Member

Not a fan of this class name. Imagine coming in as a noob - what's MP?

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 19, 2018

Member

Also MicroProfile is a very broad term, I'd name it MicroProfileHealthChecker

* only if the new code is made subject to such option by the copyright
* holder.
*/
package fish.payara.healthcheck.mp;

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 19, 2018

Member

Please don't use this abbreviation for MicroProfile - mp is too vague a term when you look at it standalone.

path = "healthcheck-mp-configure",
description = "Configures the Microprofile Healthcheck Checker")
})
public class SetHealthcheckMPCheckerConfiguration implements AdminCommand {

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 19, 2018

Member

SetMicroProfileHealthCheckerConfiguration

* only if the new code is made subject to such option by the copyright
* holder.
*/
package fish.payara.healthcheck.mp;

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 19, 2018

Member

See later comment

healthcheck.description.stuckThreads=Provides thread name, id and stack trace for requests which reach over defined threshold values
healthcheck.description.MPhealthcheck=Checks that all instances are responding to Microprofile Healthcheck requests with an UP response

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 19, 2018

Member

Don't think block capitalisation of up is necessary here

This comment has been minimized.

@Cousjava

Cousjava Sep 19, 2018

Member

The MP Health spec defines it as UP with capitalisation.

@@ -85,6 +85,7 @@
<module>microprofile</module>
<module>security-oauth2</module>
<module>security-openid</module>
<module>healthcheck-checker</module> <!-- as it depends on microprofile -->

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 19, 2018

Member

Comment not necessary

@@ -109,6 +111,7 @@
private final String machineMemoryUsagePropertyName = "machineMemoryUsage";
private final String hoggingThreadsPropertyName = "hoggingThreads";
private final String stuckThreadsPropertyName = "stuckThreads";
private final String mpHealthcheckPropertyName = "mpHealthcheck";

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 19, 2018

Member

Could probably just be mpHealth - it's more accurate (the spec is actually just "MicroProfile Health")

* @author jonathan coustick
* @since 5.184
*/
@Service(name = "set-healthcheck-mp-checker-configuration")

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 19, 2018

Member

Update in alignment with comment below

@CommandLock(CommandLock.LockType.NONE)
@I18n("healthcheck.mphealthecheck.configure")
@ExecuteOn({RuntimeType.DAS, RuntimeType.INSTANCE})
@TargetType({CommandTarget.DAS, CommandTarget.STANDALONE_INSTANCE, CommandTarget.CLUSTER, CommandTarget.CLUSTERED_INSTANCE, CommandTarget.CONFIG})

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 19, 2018

Member

Should a deployment group not also be a valid target?

* @author jonathan coustick
* @since 5.184
*/
public class TimeoutHealthCheckExecutionOptions extends HealthCheckExecutionOptions {

This comment has been minimized.

@Pandrex247

Pandrex247 Sep 19, 2018

Member

Not a fan of this name either.
Surely HealthCheckTimeoutExecutionOptions? TimeoutHealthCheckExecutionOptions sounds like a method.

@Cousjava

This comment has been minimized.

Member

Cousjava commented Sep 20, 2018

requires payara/Payara_PatchedProjects#187 to be merged first

@Cousjava Cousjava changed the title from PAYARA-2479 MP health check checker to PAYARA-2479 PAYARA-3118 MP health check checker Sep 25, 2018

@Cousjava

This comment has been minimized.

Member

Cousjava commented Sep 25, 2018

Jenkins test please

@payara-ci

This comment has been minimized.

Contributor

payara-ci commented Sep 25, 2018

Quick build and test failed!

@smillidge

This comment has been minimized.

Contributor

smillidge commented Oct 1, 2018

jenkins test please

@payara-ci

This comment has been minimized.

Contributor

payara-ci commented Oct 1, 2018

Quick build and test failed!

@Pandrex247

This comment has been minimized.

Member

Pandrex247 commented Oct 4, 2018

Jenkins test please

@payara-ci

This comment has been minimized.

Contributor

payara-ci commented Oct 4, 2018

Quick build and test failed!

@Cousjava

This comment has been minimized.

Member

Cousjava commented Oct 4, 2018

Jenkins test please

@payara-ci

This comment has been minimized.

Contributor

payara-ci commented Oct 4, 2018

Quick build and test failed!

@Pandrex247

This comment has been minimized.

Member

Pandrex247 commented Oct 4, 2018

The patched projects PR is wrong - please make a new one.

@Pandrex247

This comment has been minimized.

Member

Pandrex247 commented Oct 29, 2018

Jenkins test please

@payara-ci

This comment has been minimized.

Contributor

payara-ci commented Oct 29, 2018

Quick build and test failed!

@Pandrex247

This comment has been minimized.

Member

Pandrex247 commented Oct 29, 2018

Jenkins test please

@Cousjava

This comment has been minimized.

Member

Cousjava commented Oct 29, 2018

Jenkins test please

@payara-ci

This comment has been minimized.

Contributor

payara-ci commented Oct 29, 2018

Quick build and test passed!

1 similar comment
@payara-ci

This comment has been minimized.

Contributor

payara-ci commented Oct 29, 2018

Quick build and test passed!

@Cousjava

This comment has been minimized.

Member

Cousjava commented Oct 29, 2018

@Pandrex247 Can this be merged now?

@Cousjava Cousjava removed the DO NOT MERGE label Oct 29, 2018

@Cousjava Cousjava merged commit 4d4d4c2 into payara:master Oct 29, 2018

3 checks passed

Payara Quick Build Payara quick build passed!
Details
Payara Quick Build and Test Quick build and test passed!
Details
Payara Quick Test Payara quick tests passed!
Details

Cousjava added a commit to Cousjava/Payara that referenced this pull request Oct 30, 2018

Merge pull request payara#3125 from Cousjava/PAYARA-2479-health-check…
…-checker

PAYARA-2479 PAYARA-3118 MP health check checker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment