FISH-315 Implement New Notifier API #4884

MattGill98 · 2020-09-11T16:29:54Z

Description

This is a major refactor of the notification service. The intention is to allow modules depending solely on the internal-api module to send notifications and implement custom notifiers without either having knowledge of the notification service itself. Extra functionality / modifications include:

PayaraExecutorService is now used across all notifiers, rather than each notifier implementing their own pooling.
It's possible to implement dynamic notifiers without domain.xml that are always enabled.
Implementing custom notifiers no longer requires implementations of: MessageImpl, MessageQueue, NotificationEvent, NotificationEventFactory, NotifierConfigurationExecutionOptions, NotifierConfigurationExecutionOptionsFactory, NotifierExecutionOptions, NotifierExecutionOptionsFactory.
New GlassFishStubBean allows copying ConfigBeanProxy objects to be referenced statically (respect commands run without the dynamic flag).

Important Info

Blockers

N/A

Testing

New tests

Testing Performed

Manual testing of CDI eventbus notifier with the health check and request tracing services.

Documentation

Not yet, TBD

Notes for Reviewers

The intention is for the notifiers left out of this update to be moved into a different repository by having them depend on an internal-api artifact. Reviewing should keep this in mind - if it's not properly decoupled and version independent it doesn't work.

This reverts commit 2a0b89b. The reverted commit breaks a websocket TCK test. QACI-406 will endeavour to fix this test without rolling back functionality.

Signed-off-by: Matthew Gill <[email protected]>

A new notification module exists alongside the original notifier project, which it was copied from. This module has then been modified to incorporate the new API - which is implemented in a custom example module as well as the new log notifier. The notifiers can now be referenced by name rather than via a core enum. This makes them more modular as services like the request tracing service can discover the notifiers via HK2, and not through a centrally registered enum. Notifications can now be sent programmatically through HK2 topics. Notifiers without any configuration can now be implemented, which allows dynamic notifiers to be created by users. There is also now much less boilerplate required to implement new services, as each notifier listens for the same event type. No factories are required to be implemented by the API user. The notification get commands have also been improved to better discover the notifier names, and to print better information when used from the console. Issues left to solve: - Some dynamic flags may not work - Notifier lifecycles aren't fully controlled (when are the initialize methods called? - Performance hasn't been tested - No queues or thread pools are implemented for notifiers - Exceptions aren't handled in notifiers - Hasn't been tested in a clustered setup Signed-off-by: Matthew Gill <[email protected]>

Redesigned the notification loop to use the PayaraExecutorService. This prevents notifications with significant delay from running in the notifying thread. Signed-off-by: Matthew Gill <[email protected]>

The notifier bootstrap methods were failing if a configuration was required as it wasn't being configured correctly. Deleted unused classes. Signed-off-by: Matthew Gill <[email protected]>

Fixed the notifier implementation so that the --dynamic flag functionality is respected. This uses a new proxy bean that duplicates the current configuration and can be referenced with a lower cost. This can simply be recreated each time a dynamic configuration change is made. Signed-off-by: Matthew Gill <[email protected]>

Updated the request tracing service to use the new notifier API. Signed-off-by: Matthew Gill <[email protected]>

Updated the health check service to use the new notifier API. Signed-off-by: Matthew Gill <[email protected]>

Bugs were present, so due to time the lazy loading needs removing. Signed-off-by: Matthew Gill <[email protected]>

An empty whitelist should count as no notifiers. Signed-off-by: Matthew Gill <[email protected]>

Signed-off-by: Matthew Gill <[email protected]>

Make sure HK2 Events are enabled in notification service constructor. Signed-off-by: Matthew Gill <[email protected]>

Removed the old notification service and renamed the new notification service. The current notifiers still need revamping. Signed-off-by: Matthew Gill <[email protected]>

Signed-off-by: Matthew Gill <[email protected]>

Updated the Audit service to use the new notifier API. Signed-off-by: Matthew Gill <[email protected]>

Updated the JMX monitoring service to use the new notifier API Signed-off-by: Matthew Gill <[email protected]>

Signed-off-by: Matthew Gill <[email protected]>

The API depended on some core notification services packages which it didn't use. Signed-off-by: Matthew Gill <[email protected]>

The admin console was broken by the notifier changes, so this change makes all the relevant updates to get it working again. Signed-off-by: Matthew Gill <[email protected]>

The new Notifier implementation had strayed from the original API (so the Payara API wouldn't work). This change returns the original functionality. Signed-off-by: Matthew Gill <[email protected]>

jbee

This is a big step in the right direction getting rid of a lot of ceremony while generalising and simplifying the concept and API of notifications. A lot of code could be deleted 👍 . Glad you also updated all the command names to follow the currently used naming conversions with get- and set-.

The configuration classes and commands around them to me seem the least mature which is due to the limitation HK2 style configurations. My hopes were that we could make the configuration even more generic so less code would need virtual duplication. I highlight how HK2 config is cause of ceremony and duplication as I think this is equally the case in many other areas of the server and improvements to this concept would greatly improve what can be done and reduce what needs to be written to do so.

Mostly general, API related comments...

Topic<PayaraNotification> to me should be extracted to an interface so the actual implementation via HK2 topics is hidden to the sender making him not depend on HK2 APIs directly
I hope PayaraNotificationFactory isn't really needed and that the builder and message could be created just using static methods, e.g. PayaraNotificationBuilder.newBuilder().set. ... .build().
Naming: not sure I am a big fan of prefixing the API classes with Payara, it is apparent from the package already but I guess it helps to not confuse old and new API
currently a sender will need to use the internal-api common artefact. It might be worth even extracting just the notifier API to a API module on its own. @Pandrex247 WDYT?

appserver/admingui/payara-console-extras/src/main/resources/notification/notification.jsf

...ng-service/src/main/java/fish/payara/jmx/monitoring/admin/SetJMXMonitoringConfiguration.java

...n/internal-api/src/main/java/fish/payara/internal/notification/PayaraConfiguredNotifier.java

...-api/src/main/java/fish/payara/internal/notification/admin/BaseGetNotifierConfiguration.java

...-api/src/main/java/fish/payara/internal/notification/admin/BaseSetNotifierConfiguration.java

.../src/main/java/fish/payara/internal/notification/admin/NotificationServiceConfiguration.java

nucleus/common/internal-api/src/main/java/fish/payara/internal/notification/PayaraNotifier.java

...odules/notification-core/src/main/java/fish/payara/nucleus/notification/NotifierHandler.java

Currently the log notifier can be enabled repeatedly, meaning the domain.xml fills up with log notifier blocks. This commit prevents this by adding explicit checks to make sure the enabled notifier doesn't already exist. Signed-off-by: Matthew Gill <[email protected]>

It was unclear what the NotifierManager interface was for, so this commit adds better commentary. Signed-off-by: Matthew Gill <[email protected]>

This fixes 2 bugs: - When the notification service is initialised, it would initialise all notifiers regardless of whether they need initialising. - When a notifier was disabled dynamically, the configuration of the notifier wouldn't update so it wouldn't be destroyed The isEnabled() method now reads the current notifier configuration rather than the domain.xml, and destroying a bean will also cause a config reload before it shuts down. Signed-off-by: Matthew Gill <[email protected]>

Cousjava · 2020-09-18T12:36:45Z

Fails to build, failing on test with:

[ERROR] Tests run: 23, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.453 s <<< FAILURE! - in fish.payara.nucleus.microprofile.config.spi.PayaraConfigTest
[ERROR] convertedValuesAreCached(fish.payara.nucleus.microprofile.config.spi.PayaraConfigTest)  Time elapsed: 0.1 s  <<< FAILURE!
java.lang.AssertionError: Run 8, value is not cached expected:<2> but was:<4>
	at fish.payara.nucleus.microprofile.config.spi.PayaraConfigTest.assertValue(PayaraConfigTest.java:252)
	at fish.payara.nucleus.microprofile.config.spi.PayaraConfigTest.assertCachedValue(PayaraConfigTest.java:243)
	at fish.payara.nucleus.microprofile.config.spi.PayaraConfigTest.convertedValuesAreCached(PayaraConfigTest.java:105)

When the notification service is disabled, none of the notifiers were being shut down. They are now shutdown, and the destroy() method called if any notifiers were available. Signed-off-by: Matthew Gill <[email protected]>

MattGill98 · 2020-09-18T13:04:16Z

jenkins test please

jbee

Looks good. Have not run any testing as it is my understanding that you mostly wanted conceptual feedback. Correct me if I am wrong here.

jbee · 2020-09-18T13:21:11Z

...odules/notification-core/src/main/java/fish/payara/nucleus/notification/NotifierHandler.java

    protected void bootstrap() {
+        // Set the configuration before bootstrapping the notifier
+        if (config != null) {
+            PayaraConfiguredNotifier.class.cast(notifier).setConfiguration(config);


No, would need to checkout the codebase and browse around this a little to get a better understanding of the relations involved. But as long as we are aware of this I think we could also just give it some time and at some point in the future a batter way becomes apparent to us - then we just have to also do the change :)

Cousjava

Fails for me, with error:

GRIZZLY0200: Service exception
java.lang.NullPointerException
        at java.util.Collections$UnmodifiableCollection.<init>(Collections.java:1028)
        at java.util.Collections$UnmodifiableList.<init>(Collections.java:1304)
        at java.util.Collections.unmodifiableList(Collections.java:1289)
        at fish.payara.internal.notification.PayaraNotification.<init>(PayaraNotification.java:77)
        at fish.payara.internal.notification.PayaraNotificationBuilder.build(PayaraNotificationBuilder.java:140)
        at fish.payara.nucleus.requesttracing.RequestTracingService.processTraceEnd(RequestTracingService.java:520)
        at fish.payara.nucleus.requesttracing.RequestTracingService.endTrace(RequestTracingService.java:463)

Which occurred by enabled notification, enabling request tracing with timeout of 300 milliseconds, including non-applications, and store longest traces; then click around in the admin console a couple of times.

Signed-off-by: Matthew Gill <[email protected]>

The subject wasn't being printed in the messages. Signed-off-by: Matthew Gill <[email protected]>

MattGill98 · 2020-09-18T15:55:56Z

@Cousjava has reported that request tracing sends more notifications than before. We're both investigating

jbee · 2020-09-18T16:44:13Z

request tracing sends more notifications than before. We're both investigating

I would not be surprised if this is more correct than before - keep an open mind for this possibility ;)

Before this change, one thread ran for the notification service and polled each notifier to handle one notification at a time, before waiting 50ms. This may not keep up with the speed at which the notification queue fills up, and notifiers may blocke each other. After this change, each notifier has it's own thread, which will wait indefinitely on the queue to fill up on the take() method, and then reschedule itself every 5ms. This will be much more efficient in handling notifications, with hopefully a minimal CPU impact. Signed-off-by: Matthew Gill <[email protected]>

MattGill98 · 2020-09-21T09:43:45Z

jenkins test please

...odules/notification-core/src/main/java/fish/payara/nucleus/notification/NotifierHandler.java

Cousjava

Saving the status of notifiers in the admin console does not work.

MattGill98 · 2020-09-21T11:39:54Z

Nice spot 👍 Just modified the API to help reduce the risk of incorrect extension (as I did). Testing and will push ASAP.

The getNotifierProperties method was prone to incorrect extension, which was causing admin console configuration issues to arise. This change fixes these errors by removing the requirement to implement this method, and fixes the remaining admin console issues. Signed-off-by: Matthew Gill <[email protected]>

Signed-off-by: Matthew Gill <[email protected]>

MattGill98 · 2020-09-21T12:34:46Z

jenkins test please

Cousjava · 2020-09-21T13:21:00Z

Fails to build:

[ERROR] Failed to execute goal on project notification-email: Could not resolve dependencies for project fish.payara.server.internal.packager:notification-email:distribution-fragment:5.2020.5-SNAPSHOT: Could not find artifact fish.payara.server.internal.payara-appserver-modules:notification-email-core:jar:5.2020.5-SNAPSHOT

MattGill98 · 2020-09-21T13:24:07Z

Which module is that in? I’ve built with BuildExtras and not come across that?

Moved the example notification module into nucleus, and removed the remaining packager modules from the build lifecycle. Signed-off-by: Matthew Gill <[email protected]>

MattGill98 · 2020-09-21T14:02:56Z

jenkins test please

Cousjava

All seems to work for me

MattGill98 · 2020-09-21T15:25:28Z

Thanks @jbee and @Cousjava!

MattGill98 added 23 commits September 4, 2020 12:00

Revert "FISH-31 Upgrade Grizzly to 2.4.4.payara-p3"

4f9393a

This reverts commit 2a0b89b. The reverted commit breaks a websocket TCK test. QACI-406 will endeavour to fix this test without rolling back functionality.

Disable ServerLogTest until Grizzly upgrade

baf8c00

Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Add Thread Pooling to Notifiers

f95d3df

Redesigned the notification loop to use the PayaraExecutorService. This prevents notifications with significant delay from running in the notifying thread. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Fix Notifier Initialisation

053e5ed

The notifier bootstrap methods were failing if a configuration was required as it wasn't being configured correctly. Deleted unused classes. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Request Tracing Use New Notifier API

b1df406

Updated the request tracing service to use the new notifier API. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 HealthCheck Use New Notifier API

12e05bb

Updated the health check service to use the new notifier API. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Remove Lazy Notifier Loading

5205c62

Bugs were present, so due to time the lazy loading needs removing. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Correct Whitelist Behaviour

3d51be0

An empty whitelist should count as no notifiers. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Fix Notifier Queue Threading

bfddeae

Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Javadoc Notifier APIs

50096f0

Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Ensure HK2 Topic Distribution

85a4e79

Make sure HK2 Events are enabled in notification service constructor. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Replace Old Notification Service

83f40ce

Removed the old notification service and renamed the new notification service. The current notifiers still need revamping. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Update CDI EventBus Notifier to use new API

f29021e

Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Update Eventbus Notifier to Use New API

0d2007f

Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Update JMS Notifier to Use New API

add0d9c

Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Audit Service Use New Notifier API

41309d0

Updated the Audit service to use the new notifier API. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 JMX Monitoring Service Use New Notifier API

10c6014

Updated the JMX monitoring service to use the new notifier API Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Removed extra notifiers from build

12146e2

Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Fix OSGI Error on Loading Payara-API

b0dcaba

The API depended on some core notification services packages which it didn't use. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Fix Admin Console Notifier Integrations

3b12e2b

The admin console was broken by the notifier changes, so this change makes all the relevant updates to get it working again. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Fix CDI Eventbus API

1640ee1

The new Notifier implementation had strayed from the original API (so the Payara API wouldn't work). This change returns the original functionality. Signed-off-by: Matthew Gill <[email protected]>

MattGill98 requested review from jbee, Cousjava and MeroRai September 11, 2020 16:29

MattGill98 self-assigned this Sep 11, 2020

MeroRai requested a review from dmatej September 14, 2020 08:11

jbee suggested changes Sep 14, 2020

View reviewed changes

MattGill98 added 2 commits September 18, 2020 13:12

FISH-315 Expand NotifierManager Javadoc

4f3edf3

It was unclear what the NotifierManager interface was for, so this commit adds better commentary. Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Disable Notification Service Properly

f215290

When the notification service is disabled, none of the notifiers were being shut down. They are now shutdown, and the destroy() method called if any notifiers were available. Signed-off-by: Matthew Gill <[email protected]>

MattGill98 requested a review from jbee September 18, 2020 13:04

jbee approved these changes Sep 18, 2020

View reviewed changes

Cousjava suggested changes Sep 18, 2020

View reviewed changes

MattGill98 added 2 commits September 18, 2020 15:41

FISH-315 Fix NPE in Notification Initialisation

5b58c24

Signed-off-by: Matthew Gill <[email protected]>

FISH-315 Log Notifier Print Subject

e6fc19f

The subject wasn't being printed in the messages. Signed-off-by: Matthew Gill <[email protected]>

MattGill98 requested a review from Cousjava September 18, 2020 15:55

MattGill98 added 2 commits September 21, 2020 10:32

Merge branch 'master' into FISH-315

e29d3f4

Cousjava reviewed Sep 21, 2020

View reviewed changes

...odules/notification-core/src/main/java/fish/payara/nucleus/notification/NotifierHandler.java Show resolved Hide resolved

Cousjava suggested changes Sep 21, 2020

View reviewed changes

MattGill98 added 2 commits September 21, 2020 13:20

FISH-315 Fix Test Notifier Console Buttons

559241a

Signed-off-by: Matthew Gill <[email protected]>

MattGill98 requested a review from Cousjava September 21, 2020 12:34

FISH-315 Fix Module Structure

9373ae7

Moved the example notification module into nucleus, and removed the remaining packager modules from the build lifecycle. Signed-off-by: Matthew Gill <[email protected]>

Cousjava approved these changes Sep 21, 2020

View reviewed changes

MattGill98 merged commit 35f2f2d into payara:master Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FISH-315 Implement New Notifier API #4884

FISH-315 Implement New Notifier API #4884

MattGill98 commented Sep 11, 2020 •

edited

Loading

jbee left a comment

Cousjava commented Sep 18, 2020

MattGill98 commented Sep 18, 2020

jbee left a comment

jbee Sep 18, 2020

Cousjava left a comment

MattGill98 commented Sep 18, 2020

jbee commented Sep 18, 2020 •

edited

Loading

MattGill98 commented Sep 21, 2020

Cousjava left a comment

MattGill98 commented Sep 21, 2020

MattGill98 commented Sep 21, 2020

Cousjava commented Sep 21, 2020

MattGill98 commented Sep 21, 2020

MattGill98 commented Sep 21, 2020

Cousjava left a comment

MattGill98 commented Sep 21, 2020

FISH-315 Implement New Notifier API #4884

FISH-315 Implement New Notifier API #4884

Conversation

MattGill98 commented Sep 11, 2020 • edited Loading

Description

Important Info

Blockers

Testing

New tests

Testing Performed

Documentation

Notes for Reviewers

jbee left a comment

Choose a reason for hiding this comment

Cousjava commented Sep 18, 2020

MattGill98 commented Sep 18, 2020

jbee left a comment

Choose a reason for hiding this comment

jbee Sep 18, 2020

Choose a reason for hiding this comment

Cousjava left a comment

Choose a reason for hiding this comment

MattGill98 commented Sep 18, 2020

jbee commented Sep 18, 2020 • edited Loading

MattGill98 commented Sep 21, 2020

Cousjava left a comment

Choose a reason for hiding this comment

MattGill98 commented Sep 21, 2020

MattGill98 commented Sep 21, 2020

Cousjava commented Sep 21, 2020

MattGill98 commented Sep 21, 2020

MattGill98 commented Sep 21, 2020

Cousjava left a comment

Choose a reason for hiding this comment

MattGill98 commented Sep 21, 2020

MattGill98 commented Sep 11, 2020 •

edited

Loading

jbee commented Sep 18, 2020 •

edited

Loading