Refactor notification center to use generics. #288

mikecdavis · 2019-04-30T19:06:34Z

Summary

Define new Notification interface to mark supported notification models.
Define new generic NotificationHandler<T> interface
Define NotifcationManager<T> class to own specific class of notifications.
Refactor NotificationCenter to hold a map of NotificationManager instances.
Define explicit ActivateNotification class
Refactor ActivateNotificationListener to implement NotificationHandler<ActivateNotification>
Define explicit TrackNotification class
Refactor TrackNotificationListener to implement NotificationHandler<TrackNotification>

The current patterns surrounding the NotificationCenter did not extend well and relied on Object... array arguments into the NotificationListener interface. This resulted in the need for dedicated abstract classes, enums and helper methods to be implemented for each notification. Moving to a generic NotificationHandler<T> interface and NotifcationManager<T> means that new notifications can be defined simply by implementing a new Notification model which is type safe, discoverable and self describing.

Test plan

Unit and integration testing along with the compatibility suite.

coveralls · 2019-04-30T19:13:39Z

Pull Request Test Coverage Report for Build 997

130 of 145 (89.66%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 89.79%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java	42	57	73.68%

Totals
Change from base Build 987:	0.2%
Covered Lines:	2911
Relevant Lines:	3242

💛 - Coveralls

mikecdavis · 2019-04-30T20:47:26Z

This is failing compatibility-suite changes since I removed the explicit DecisionNotificationListener class and methods for a more extensible pattern. This should be resolved with updates to the test-app.

aliabbasrizvi

Looks good.

We will have to update documentation here: https://docs.developers.optimizely.com/full-stack/docs/register-notification-listeners

Work is tracked in OASIS-4583

aliabbasrizvi · 2019-04-30T21:38:06Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationManager.java

+
+    public int addHandler(NotificationHandler<T> newHandler) {
+
+        // Prevent registering a duplication listener.


nit. duplicate.

Would it make sense to flip the map to be Map<NotificationHandler, Int>?

mikeproeng37 · 2019-05-01T17:16:11Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

+    }
+
+    /**
+     * Convenience method for adding TrackNotification Handlers


adding DecisionNotification handlers

mikeproeng37 · 2019-05-01T17:17:49Z

core-api/src/main/java/com/optimizely/ab/notification/ActivateNotificationListener.java

 import com.optimizely.ab.event.LogEvent;

 import javax.annotation.Nonnull;
+import java.rmi.activation.ActivateFailedException;


Is this needed? I am not seeing it used below

mikeproeng37 · 2019-05-01T17:21:57Z

core-api/src/main/java/com/optimizely/ab/notification/Notification.java

+package com.optimizely.ab.notification;
+
+/**
+ * Interface used to identify supported annotations


Supported notifications?

loganlinn

If we are going to identify the type of notification by its Class object, we should could do a little more to restrict the type hierarchy. Can we make the Notification implementations declared here final? it wouldn't make sense AFAICT to want to/be able to extend those three classes.

Also, since there are a closed set of notification types that the system supports, the Notification is ideally not public.

I think the subtleties of restricted type hierarchy is more clearly conveyed if the type family is isolated to a single file or package.

loganlinn

We should try to add comments everywhere there’s a @Deprecated annotation to describe the canonical/preferred alternative

loganlinn · 2019-05-01T18:28:46Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationManager.java

+
+    public int addHandler(NotificationHandler<T> newHandler) {
+
+        // Prevent registering a duplication listener.


Would it make sense to flip the map to be Map<NotificationHandler, Int>?

loganlinn · 2019-05-01T18:30:14Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationManager.java

+
+    private static final Logger logger = LoggerFactory.getLogger(NotificationManager.class);
+
+    private final Map<Integer, NotificationHandler<T>> handlers = new HashMap<>();


Use LinkedHashMap so there’s consistent order in handler invocation?

loganlinn · 2019-05-01T18:32:05Z

core-api/src/main/java/com/optimizely/ab/notification/ActivateNotificationListener.java

    }

+    @Override
+    public final void notify(ActivateNotification message) {


If it doesn’t break public API compatibility, it would nice to not be overloading Object.notify anymore...

Ah, good point I can change that. Suggestion? handle? since the new interface is NotificationHandler<T>

loganlinn · 2019-05-01T18:37:46Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationManager.java

+    private final AtomicInteger counter;
+
+    public NotificationManager(AtomicInteger counter) {
+        this.counter = counter;


Seems odd for this constructor to be public; could use a null check if it remains public.

Made package private

loganlinn · 2019-05-01T18:43:09Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

+    @SuppressWarnings("unchecked")
+    public <T> NotificationManager<T> getNotificationManager(Class<? extends Notification> clazz) {
+        NotificationManager<T> newManager = new NotificationManager<>(counter);
+        NotificationManager<T> manager = (NotificationManager<T>) notifierMap.putIfAbsent(clazz, newManager);


Use computeIfAbsent() to avoid creating throw-away instance when manager exists? Alternatively, the set of notification types is (all likely will remain) trivially small, so could just pre-populate the map.

loganlinn · 2019-05-01T18:52:29Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

-        decisionListenerHolder.clear();
-        for (NotificationType type : NotificationType.values()) {
-            clearNotificationListeners(type);
+        for (NotificationManager<?> manager : notifierMap.values()) {


Minor: just clearing the entire notifierMap would be more consistent with the current scheme of lazy population used with putIfAbsent

Update: We're now creating a unmodifiable map containing all possible NotificationManager types, so clearing the individual managers is required.

loganlinn · 2019-05-01T18:54:50Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

+    public void send(Notification notification) {
+        NotificationManager handler = notifierMap.get(notification.getClass());
+        if (handler == null) {
+            return;


This seems worth logging a warn or error

Replaced with an OptimizelyRuntimeException since "we" control the notifications and should be catching this in our CI

loganlinn · 2019-05-01T18:56:54Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationHandler.java

+package com.optimizely.ab.notification;
+
+public interface NotificationHandler<T> {
+    void notify(T message);


Left a comment elsewhere but I’ll leave one here too: not a great name for any Java method as every Object has notify() for very different purpose.

Still need to do this.... sticking with "handle" since no other option has been proposed :)

mikeproeng37 · 2019-05-01T22:02:42Z

Passing custom build: https://travis-ci.com/optimizely/fullstack-sdk-compatibility-suite/jobs/197018131

mikecdavis · 2019-05-02T05:56:00Z

@loganlinn as always, thanks for the feedback:)

I ended up defining an unmodifiable Map of allowable notification types to control access in lieu of the Notification interface which proved difficult to constrain. So instead of lazily instantiating the set of NotificationManagers we're defining those up front to gate registration and notification sending and to fail fast during development of new notifications.

mikeproeng37 · 2019-05-02T18:09:15Z

Passing build after new changes here: https://travis-ci.com/optimizely/fullstack-sdk-compatibility-suite/builds/110421443

Mike Davis added 3 commits April 30, 2019 09:56

Refactor notifications to use generics.

e1a4af2

Add more unit tests and additional refactor.

fd85518

A few more changes.

2e90da0

mikecdavis requested review from aliabbasrizvi, loganlinn and mikeproeng37 April 30, 2019 19:06

Mike Davis added 3 commits April 30, 2019 13:13

Fix doc string.

accc36d

Add coverage to Optimizely

0e42d13

Add coverage to NotificationCenter

fa400dd

mikecdavis mentioned this pull request Apr 30, 2019

Implement default ProjectConfigManager. #287

Merged

aliabbasrizvi approved these changes Apr 30, 2019

View reviewed changes

mikeproeng37 reviewed May 1, 2019

View reviewed changes

loganlinn reviewed May 1, 2019

View reviewed changes

Allow nulls per the compat-suite test and backwards compatibility.

f0b63de

Mike Davis added 3 commits May 1, 2019 17:41

Address additional PR feedback.

6f9a6dd

Update doc string.

cd0d49a

Refactor to notify,

9eb81fc

mikeproeng37 merged commit 4e28052 into master May 2, 2019

mikecdavis mentioned this pull request May 6, 2019

Catch errors thrown while sending notifications. #295

Merged

mikecdavis deleted the mikecdavis/refactor-notification-center branch May 8, 2019 19:01


		public int addHandler(NotificationHandler<T> newHandler) {

		// Prevent registering a duplication listener.


		private static final Logger logger = LoggerFactory.getLogger(NotificationManager.class);

		private final Map<Integer, NotificationHandler<T>> handlers = new HashMap<>();

Refactor notification center to use generics. #288

Refactor notification center to use generics. #288

Uh oh!

Conversation

mikecdavis commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coveralls commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 997

💛 - Coveralls

Uh oh!

mikecdavis commented Apr 30, 2019

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loganlinn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loganlinn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 commented May 1, 2019

Uh oh!

mikecdavis commented May 2, 2019

Uh oh!

mikeproeng37 commented May 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mikecdavis commented Apr 30, 2019 •

edited

Loading

coveralls commented Apr 30, 2019 •

edited

Loading

loganlinn left a comment •

edited

Loading