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

PAYARA-1896 Unified asadmin command to set health check notifier configuration #3663

Merged
merged 5 commits into from Feb 8, 2019

Conversation

Projects
None yet
4 participants
@jbee
Copy link
Contributor

jbee commented Jan 29, 2019

This PR is only one part of the effort of PAYARA-1896 split for easier review.

Goal was to combine the various asadmin commands to set individual health check notifier configurations into one command. To not break existing commands these were deprecated and a new command set-healthcheck-service-notifier-configuration was added.

The working principle of the new service is largely based on the BaseHealthCheckNotifierConfigurer that was the base class for the various specific commands.

The new command has an additional parameter to specify the type of notifier the setting should apply to. The list of possible values is based on the enum NotifierType. The code is entirely written so newly added types of notifiers do not require changes to this service/command.

Like the BaseHealthCheckNotifierConfigurer this implementation uses a default value for noisy resolved from the corresponding NotifierConfiguration. Unlike the
BaseHealthCheckNotifierConfigurer implementation this value is not resolved by running another command and resolving the flag from the ActionReport. Instead the corresponding NotifierConfiguration instance is directly resolved from the NotificationServiceConfiguration so that the flag can be read directly using NotifierConfiguration#getNoisy(). This allows services to be less entangled and reliant upon each other which should result in a more stable behaviour.

The new command also prints feedback on the changes to the config. It will only apply and report actual changes, that is a setting changing from true to false or vice versa.

Example session:

Command set-healthcheck-service-notifier-configuration executed successfully.
asadmin> set-healthcheck-service-notifier-configuration --enabled=false --notifier=jms --noisy=true
jms.noisy was false set to true

Command set-healthcheck-service-notifier-configuration executed successfully.
asadmin> set-healthcheck-service-notifier-configuration --enabled=true --notifier=jms --noisy=false
jms.enabled was false set to true
jms.noisy was true set to false

Command set-healthcheck-service-notifier-configuration executed successfully.
asadmin> set-healthcheck-service-notifier-configuration --enabled=true --notifier=jms --noisy=false
Command set-healthcheck-service-notifier-configuration executed successfully.
asadmin> 

@jbee jbee self-assigned this Jan 29, 2019

@mulderbaba mulderbaba added this to the 5.191 milestone Jan 29, 2019

@mulderbaba

This comment has been minimized.

Copy link
Member

mulderbaba commented Jan 29, 2019

jenkins test

@smillidge smillidge changed the title Unified asadmin command to set health check notifier configuration Unified asadmin command to set health check notifier configuration PAYARA-1896 Jan 29, 2019

@smillidge

This comment has been minimized.

Copy link
Contributor

smillidge commented Jan 29, 2019

Can you remember to use the JIRA number in commit comments and PR names. That way JIRA picks up all the changes and associates with your PR.

@jbee jbee requested review from Cousjava and Pandrex247 Jan 30, 2019

report.setExtraProperties(extraProperties);
}
try {
notifierType = NotifierType.valueOf(notifierName.toUpperCase());

This comment has been minimized.

@jbee

jbee Jan 30, 2019

Author Contributor

I intentionally chose to not use acceptableValues to allow the command adopt automatically to changes to the enum.

@jbee

This comment has been minimized.

Copy link
Contributor Author

jbee commented Jan 30, 2019

jenkins test please

@mulderbaba mulderbaba changed the title Unified asadmin command to set health check notifier configuration PAYARA-1896 PAYARA-1896 Unified asadmin command to set health check notifier configuration Jan 31, 2019

@Pandrex247
Copy link
Member

Pandrex247 left a comment

Code Styling

* {@link NotificationServiceConfiguration} to use its {@link NotifierConfiguration#getNoisy()} as automatic fallback
* setting.
*
* @author jan

This comment has been minimized.

@Pandrex247

Pandrex247 Feb 4, 2019

Member

Full name please

final Notifier notifier = selectByType(Notifier.class, config.getNotifierList());
try {
if (notifier == null) {
ConfigSupport.apply((SingleConfigCode<HealthCheckServiceConfiguration>) proxy -> {

This comment has been minimized.

@Pandrex247

Pandrex247 Feb 4, 2019

Member

configProxy or healthCheckServiceConfigProxy please

}

private <T> T selectByType(Class<? super T> commonInterface, List<T> candidates) {
for (T e : candidates) {

This comment has been minimized.

@Pandrex247

Pandrex247 Feb 4, 2019

Member

Don't use single character variables please

@jbee

This comment has been minimized.

Copy link
Contributor Author

jbee commented Feb 5, 2019

jenkins test please

@Pandrex247 Pandrex247 merged commit a295df0 into payara:master Feb 8, 2019

1 check passed

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

jbee added a commit to jbee/Payara that referenced this pull request Feb 14, 2019

Merge pull request payara#3663 from jbee/health-check-align-asadmin-c…
…ommands

PAYARA-1896 Unified asadmin command to set health check notifier configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.