Fix: Added notificationRegistry to make sure that odpSettings updates #501

mnoman09 · 2023-01-11T17:40:40Z

Summary

NotificationRegistry is added to create default NotificationCenter for internal use for updating ODPConfigSetting.
This is to handle a corner case that if in any case user passes different notificationCenter to Optimizely and ProjectconfigManager then the Notification of UpdateProjectConfig will not trigger because its handler is getting added in the main NotificationCenter object.
This will make sure UpdateProjectConfig will update ODPConfigSetting everytime irrespective of different NotificationCenter passed by user.
Added getCachedConfig method in ProjectconfigManager to return ProjectConfig without await.

NOTE: breaking change
Anyone who is implementing custom ProjectConfigManager should now have to implement getCachedConfig and getSdkKey .

Test plan

Added additional unit test to test NotificationRegistry
All tests should pass.

Issue

FSSDK-8769

…call upon UpdateConfig

jaeopt

Looks good. Some changes suggested.

jaeopt · 2023-01-11T18:02:47Z

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

@@ -127,7 +128,12 @@ private Optimizely(@Nonnull EventHandler eventHandler,
            if (getProjectConfig() != null) {
                updateODPSettings();
            }
-            addUpdateConfigNotificationHandler(configNotification -> { updateODPSettings(); });
+            if (projectConfigManager != null) {
+                NotificationRegistry.getNotificationCenter(projectConfigManager.getSDKKey()).


This can be confusing with with public NotificationCenter. What about changing names to
NotificationRegistry.getInternalNotificationCenter or some other names to avoid confusion?
@msohailhussain What do you think?

jaeopt · 2023-01-11T18:19:58Z

core-api/src/main/java/com/optimizely/ab/config/ProjectConfigManager.java

@@ -23,5 +23,7 @@ public interface ProjectConfigManager {
     * @return ProjectConfig
     */
    ProjectConfig getConfig();
+
+    String getSDKKey();


Can we get sdkKey via getConfig().getSDKKey() instead of adding the new inferface?
This will be convenient but we want to avoid any breaking changes with this new method here.

see this comment. It will make sure that even when the config is not fetched, notificationCenter will contain the handler, which updates the ODPConfig.

@mnoman09 Got it! Can we add a default implementation of this method (returning null), so no changes required for existing implementations. They'll all share the same internal notificationCenter for "null" sdkKey (until they fix it), which looks safe to me.
I believe the default interface method is supported in Java8 and also supported in android.

jaeopt · 2023-01-11T18:25:29Z

core-httpclient-impl/src/main/java/com/optimizely/ab/config/HttpProjectConfigManager.java

@@ -73,11 +74,13 @@ private HttpProjectConfigManager(long period,
                                     String datafileAccessToken,
                                     long blockingTimeoutPeriod,
                                     TimeUnit blockingTimeoutUnit,
-                                     NotificationCenter notificationCenter) {
+                                     NotificationCenter notificationCenter,
+                                     String sdkKey) {


Can we also avoid this change as well if we can get sdkKey via ProjectConfig?

Same as above. We need to have sdkKey pass in here, to make sure that the when we call getSDKKey on projectConfigManager then we won't get it null in case if it is http config Manager.

mnoman09 · 2023-01-11T19:26:33Z

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

@@ -127,7 +128,12 @@ private Optimizely(@Nonnull EventHandler eventHandler,
            if (getProjectConfig() != null) {
                updateODPSettings();
            }
-            addUpdateConfigNotificationHandler(configNotification -> { updateODPSettings(); });
+            if (projectConfigManager != null) {
+                NotificationRegistry.getNotificationCenter(projectConfigManager.getSDKKey()).


@jaeopt we are getting sdkKey from projectConfigManager instead of getProjectConfig() because of Asynchronous call of httpConfigManager it might not able to add this NotificationHandler when projectConfig is not fetched.

Added default implementation of getSDKKey in ProjectConfigManager

jaeopt

All changes look great. A couple of more changes suggested.

jaeopt · 2023-01-12T21:30:13Z

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

@@ -153,6 +159,7 @@ public void close() {
        tryClose(eventProcessor);
        tryClose(eventHandler);
        tryClose(projectConfigManager);
+        NotificationRegistry.clearNotificationCenterRegistry();


Is this safe to clear all notificationCenters? We can clear one for the current sdkKey. @msohailhussain also shared concern about memory leak when we close a notificationCenter, which may be addressed together when we clean it up.

jaeopt · 2023-01-12T21:36:34Z

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

@@ -127,7 +128,12 @@ private Optimizely(@Nonnull EventHandler eventHandler,
            if (getProjectConfig() != null) {


It looks like my previous comment on this line got lost :)
We discussed this blocking "getProjectConfig" while constructing Optimizely is not desirable, so we need to make it return null if not available. @msohailhussain may have a good idea on how to fix it.

jaeopt

We may need one more change. See my comments.

jaeopt · 2023-01-17T18:31:14Z

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

@@ -124,10 +125,15 @@ private Optimizely(@Nonnull EventHandler eventHandler,

        if (odpManager != null) {
            odpManager.getEventManager().start();
-            if (getProjectConfig() != null) {
+            if (!(projectConfigManager instanceof PollingProjectConfigManager)) {


PollingProjectConfigManager can also have a preset projectConfig from a fallback datafile. In that case, we have to call updateODPSettings for the projectConfig immediately (we may not get notification later if the fallback is updated already).
I think we can keep the getProjectConfig() != null condition as before, with the change that `getProjectConfig() returns null immediately if config is not available.

getProjectConfig() != null condition is not possible without wait, with our current implementation. However, we can use the similar approach as we are using in Csharp, adding additional function of getCachedConfig() in ProjectConfigManager. Now the only concern I have, related to this approach, is that do we want to enforce its implementation or return null by default as we are doing for getSDKKey() function?
In my opinion we should enforce it because if anyone uses custom ProjectConfigManager without giving getCachedConfig implementation then they won't be able use ODP as it only gets config using this new function.

@mnoman09 Agreed. Let's make them (getSdkKey and getCachedConfig) mandatory changes and keep them in the list of breaking changes for release.

…but the default implementation of it will return null.

…heir own choice.

jaeopt

All changes look good to me!
A couple of small changes suggested - clarifications about required changes for existing custom configMangers. Android-sdk is one of them with tricky changes required.

core-api/src/main/java/com/optimizely/ab/config/ProjectConfigManager.java

core-httpclient-impl/src/main/java/com/optimizely/ab/config/HttpProjectConfigManager.java

jaeopt · 2023-01-19T23:30:55Z

@mnoman09 It'll be great if we can have a note about these breaking changes so we do not miss them when releasing a major rev.
We can use the "Unreleased" header in CHANGELOG.md like csharp-sdk - https://github.com/optimizely/csharp-sdk/blob/master/CHANGELOG.md

…Key for a particular config is always same.

…icked from projectConfig

jaeopt

LGTM - See a couple of small changes to clean up.

jaeopt · 2023-01-25T17:44:55Z

core-httpclient-impl/src/main/java/com/optimizely/ab/config/HttpProjectConfigManager.java

                sdkKey);
-
+            httpProjectManager.setSdkKey(sdkKey);


Isn't this seSdkKey is redundant? I see `sdkKey' parameter to the constructor (line 368).

This will ensure that to trigger notification user provided sdkKey will always be prioritized, instead of what is available in ProjectConfig. This allows registry to always trigger notification even if the SDKKey is changed in the Projectconfig.

@mnoman09 If we need this extra description for proper initializing sdkKey, this may be a source of issues. If you have to call setSdkKey() for proper overriding, can we remove sdkKey from constructor?

I already removed it from the HttpProjectConfigManager constructor if that is what you are asking?

@mnoman09 looks good!

core-api/src/main/java/com/optimizely/ab/config/PollingProjectConfigManager.java

…hat to trigger the notification always user provided sdkKey will be prioritized

jaeopt

LGTM

Added notificationRegistry to make sure that odpSettings update will …

a006cf0

…call upon UpdateConfig

mnoman09 requested a review from a team as a code owner January 11, 2023 17:40

optimizely-codereview-bot assigned mnoman09 Jan 11, 2023

Updated headers and added additional checks in unittests

ed2bcbb

jaeopt requested changes Jan 11, 2023

View reviewed changes

mnoman09 commented Jan 11, 2023

View reviewed changes

NomanShoaib added 6 commits January 12, 2023 00:31

Added suppress warning

51357cf

renamed NoficationRegistry function.

f1c3d7a

Added default implementation of getSDKKey in ProjectConfigManager

reverting header change

5d791f2

reverting extra changes

01fd113

reverting extra changes

9c4f729

nit fix

92004cf

mnoman09 requested a review from jaeopt January 12, 2023 11:27

mnoman09 removed their assignment Jan 12, 2023

nit

01313a4

jaeopt requested changes Jan 12, 2023

View reviewed changes

optimizely-codereview-bot assigned mnoman09 Jan 12, 2023

mnoman09 requested a review from jaeopt January 16, 2023 09:43

mnoman09 removed their assignment Jan 16, 2023

Resolved comments

ecb12b5

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs and removed 🤖 4 hrs labels Jan 16, 2023

jaeopt requested changes Jan 17, 2023

View reviewed changes

optimizely-codereview-bot removed the 🤖 8 hrs label Jan 17, 2023

optimizely-codereview-bot assigned mnoman09 Jan 17, 2023

NomanShoaib added 2 commits January 18, 2023 19:35

Added getCachedconfig Function to return ProjectConfig without wait, …

38d735a

…but the default implementation of it will return null.

ProjectConfig getCachedConfig(); without default implementation

0deec47

mnoman09 removed their assignment Jan 18, 2023

optimizely-codereview-bot requested a review from jaeopt January 18, 2023 14:54

NomanShoaib added 2 commits January 18, 2023 19:57

Merge branch 'master' into mnoman/addNotificationRegistry

e6bbc30

Removed default implementation of getSDKkey to enforce user to make t…

43da46a

…heir own choice.

jaeopt requested changes Jan 19, 2023

View reviewed changes

core-api/src/main/java/com/optimizely/ab/config/ProjectConfigManager.java Show resolved Hide resolved

core-httpclient-impl/src/main/java/com/optimizely/ab/config/HttpProjectConfigManager.java Outdated Show resolved Hide resolved

optimizely-codereview-bot assigned mnoman09 Jan 19, 2023

mnoman09 removed their assignment Jan 25, 2023

mnoman09 requested a review from jaeopt January 25, 2023 10:54

NomanShoaib added 5 commits January 25, 2023 15:55

Resolved comments

0fe4fb3

setting Sdk key inside pollingConfigManager to make sure that the sdk…

dd6a823

…Key for a particular config is always same.

setting sdkKey in parent class

680a5bf

fix

65c150d

making sure that if the SDKKey is set by the user then it should be p…

5bc8ffd

…icked from projectConfig

jaeopt approved these changes Jan 25, 2023

View reviewed changes

Refactored and moved getSdkKey to pollingConfigManager to make sure t…

26d50ae

…hat to trigger the notification always user provided sdkKey will be prioritized

jaeopt approved these changes Jan 26, 2023

View reviewed changes

msohailhussain merged commit cbaeb85 into master Jan 26, 2023

msohailhussain deleted the mnoman/addNotificationRegistry branch January 26, 2023 22:17

opti-jnguyen mentioned this pull request Jan 27, 2023

Add Notification Registry for ODP Setting Updates optimizely/javascript-sdk#795

Merged

jaeopt mentioned this pull request May 4, 2023

[FSSDK-8795]chore: prepare for pre release 4.0.0-beta #515

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Added notificationRegistry to make sure that odpSettings updates #501

Fix: Added notificationRegistry to make sure that odpSettings updates #501

mnoman09 commented Jan 11, 2023 •

edited

Loading

jaeopt left a comment

jaeopt Jan 11, 2023

jaeopt Jan 11, 2023

mnoman09 Jan 11, 2023

jaeopt Jan 11, 2023

jaeopt Jan 11, 2023

mnoman09 Jan 12, 2023 •

edited

Loading

mnoman09 Jan 11, 2023

jaeopt left a comment

jaeopt Jan 12, 2023

jaeopt Jan 12, 2023

jaeopt left a comment

jaeopt Jan 17, 2023

mnoman09 Jan 18, 2023

jaeopt Jan 18, 2023

jaeopt left a comment

jaeopt commented Jan 19, 2023

jaeopt left a comment

jaeopt Jan 25, 2023

mnoman09 Jan 25, 2023

jaeopt Jan 25, 2023

mnoman09 Jan 26, 2023

jaeopt Jan 26, 2023

jaeopt left a comment

		@@ -127,7 +128,12 @@ private Optimizely(@Nonnull EventHandler eventHandler,
		if (getProjectConfig() != null) {

Fix: Added notificationRegistry to make sure that odpSettings updates #501

Fix: Added notificationRegistry to make sure that odpSettings updates #501

Conversation

mnoman09 commented Jan 11, 2023 • edited Loading

Summary

Test plan

Issue

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnoman09 Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

jaeopt commented Jan 19, 2023

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

mnoman09 commented Jan 11, 2023 •

edited

Loading

mnoman09 Jan 12, 2023 •

edited

Loading