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-3666 Fixes for issues found by tests for new health check as-admin commands #3798

Merged

Conversation

@jbee
Copy link
Contributor

commented Feb 26, 2019

Several issues were found by the tests (separate PR as soon as I know where they should go and how to integrate them in the build/test execution) created for new health check asadmin commands.

I annotated changes with an explanation.

@jbee jbee added the DO NOT MERGE label Feb 26, 2019

@jbee jbee self-assigned this Feb 26, 2019

@@ -208,13 +209,16 @@ public void bootstrapHealthCheck() {
/**
* Starts all notifiers that have been enable with the healthcheck service.
*/
public void bootstrapNotifierList() {
public synchronized void bootstrapNotifierList() {

This comment has been minimized.

Copy link
@jbee

jbee Feb 26, 2019

Author Contributor

I think this should be synchronized to be consistent in case of concurrent service reboots (e.g. possible by concurrent admin commands).

notifierExecutionOptionsList = new ArrayList<>();
if (configuration.getNotifierList() != null) {
for (Notifier notifier : configuration.getNotifierList()) {
ConfigView view = ConfigSupport.getImpl(notifier);
NotifierConfigurationType annotation = view.getProxyType().getAnnotation(NotifierConfigurationType.class);
notifierExecutionOptionsList.add(executionOptionsFactoryStore.get(annotation.type()).build(notifier));
NotifierExecutionOptionsFactory<Notifier> factory = executionOptionsFactoryStore.get(annotation.type());
if (factory != null) {

This comment has been minimized.

Copy link
@jbee

jbee Feb 26, 2019

Author Contributor

The factory can be null depending on prior changes to the notifier configurations and reboots occurring.

I'm not 100% sure this is the right fix. Maybe the factory should never be null but it solves the immediate problem.

@@ -300,7 +304,8 @@ public void shutdownHealthCheck() {
}

public BaseHealthCheck getCheck(String serviceName) {
return registeredTasks.get(serviceName).getCheck();
HealthCheckTask task = registeredTasks.get(serviceName);
return task == null ? null : task.getCheck();

This comment has been minimized.

Copy link
@jbee

jbee Feb 26, 2019

Author Contributor

Here NPE occurred in test scenarios if there was no task for the serviceName. Returning null allows the caller to take care of the case.

@@ -56,7 +57,7 @@
public class NotifierExecutionOptionsFactoryStore {

private Map<NotifierType, NotifierExecutionOptionsFactory> execOptionsFactoryStore =
new HashMap<NotifierType, NotifierExecutionOptionsFactory>();
new ConcurrentHashMap<NotifierType, NotifierExecutionOptionsFactory>();

This comment has been minimized.

Copy link
@jbee

jbee Feb 26, 2019

Author Contributor

If I get HK2 semantics right this map could be accessed and modified by multiple threads so it should be thread safe.

@jbee

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

jenkins test please

@arjantijms arjantijms added this to the 5.192 milestone Feb 27, 2019

@jbee jbee requested review from Pandrex247, mulderbaba, smillidge and arjantijms and removed request for mulderbaba Feb 28, 2019

@jbee jbee added payara-micro and removed DO NOT MERGE labels Feb 28, 2019

@jbee

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

jenkins test please

@arjantijms arjantijms changed the title QA-186 Fixes for issues found by tests for new health check as-admin commands PAYARA-3582 Fixes for issues found by tests for new health check as-admin commands Mar 8, 2019

@Pandrex247 Pandrex247 changed the title PAYARA-3582 Fixes for issues found by tests for new health check as-admin commands PAYARA-3666 Fixes for issues found by tests for new health check as-admin commands Mar 12, 2019

Concern answered - asadmin will prompt for missing Boolean.

@Pandrex247 Pandrex247 merged commit 039acfa into payara:master Mar 19, 2019

59 checks passed

Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/javaee-api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
@jbee

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Pandrex247 added a commit to Pandrex247/Payara that referenced this pull request Jun 12, 2019

Merge pull request payara#3798 from jbee/QA-186-new-health-check-asad…
…min-commands

PAYARA-3666 Fixes for issues found by tests for new health check as-admin commands
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.