-
Notifications
You must be signed in to change notification settings - Fork 32
Fix: Added notificationRegistry to make sure that odpSettings updates #501
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
Changes from 9 commits
a006cf0
ed2bcbb
51357cf
f1c3d7a
5d791f2
01fd113
9c4f729
92004cf
01313a4
ecb12b5
38d735a
0deec47
e6bbc30
43da46a
0fe4fb3
dd6a823
680a5bf
65c150d
5bc8ffd
26d50ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
import com.optimizely.ab.event.*; | ||
import com.optimizely.ab.event.internal.*; | ||
import com.optimizely.ab.event.internal.payload.EventBatch; | ||
import com.optimizely.ab.internal.NotificationRegistry; | ||
import com.optimizely.ab.notification.*; | ||
import com.optimizely.ab.odp.*; | ||
import com.optimizely.ab.optimizelyconfig.OptimizelyConfig; | ||
|
@@ -127,7 +128,12 @@ private Optimizely(@Nonnull EventHandler eventHandler, | |
if (getProjectConfig() != null) { | ||
updateODPSettings(); | ||
} | ||
addUpdateConfigNotificationHandler(configNotification -> { updateODPSettings(); }); | ||
if (projectConfigManager != null) { | ||
NotificationRegistry.getInternalNotificationCenter(projectConfigManager.getSDKKey()). | ||
addNotificationHandler(UpdateConfigNotification.class, | ||
configNotification -> { updateODPSettings(); }); | ||
} | ||
|
||
} | ||
} | ||
|
||
|
@@ -153,6 +159,7 @@ public void close() { | |
tryClose(eventProcessor); | ||
tryClose(eventHandler); | ||
tryClose(projectConfigManager); | ||
NotificationRegistry.clearNotificationCenterRegistry(); | ||
|
||
if (odpManager != null) { | ||
tryClose(odpManager); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
/** | ||
* | ||
* Copyright 2023, Optimizely | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package com.optimizely.ab.internal; | ||
|
||
import com.optimizely.ab.notification.NotificationCenter; | ||
|
||
import javax.annotation.Nonnull; | ||
import java.util.Map; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
|
||
public class NotificationRegistry { | ||
private final static Map<String, NotificationCenter> _notificationCenters = new ConcurrentHashMap<>(); | ||
|
||
private NotificationRegistry() | ||
{ | ||
} | ||
|
||
public static NotificationCenter getInternalNotificationCenter(@Nonnull String sdkKey) | ||
{ | ||
if (sdkKey == null) { | ||
sdkKey = ""; | ||
} | ||
|
||
NotificationCenter notificationCenter; | ||
if (_notificationCenters.containsKey(sdkKey)) { | ||
notificationCenter = _notificationCenters.get(sdkKey); | ||
} else { | ||
notificationCenter = new NotificationCenter(); | ||
_notificationCenters.put(sdkKey, notificationCenter); | ||
} | ||
return notificationCenter; | ||
} | ||
|
||
public static void clearNotificationCenterRegistry() { | ||
_notificationCenters.clear(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
/** | ||
* | ||
* Copyright 2023, Optimizely | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package com.optimizely.ab.internal; | ||
|
||
import com.optimizely.ab.notification.NotificationCenter; | ||
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
import org.junit.Assert; | ||
import org.junit.Test; | ||
|
||
import static org.junit.Assert.assertEquals; | ||
|
||
|
||
public class NotificationRegistryTest { | ||
|
||
@SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION") | ||
@Test | ||
public void getSameNotificationcenterWhenSDKkeyIsNull() { | ||
String sdkKey = null; | ||
NotificationCenter notificationCenter1 = NotificationRegistry.getInternalNotificationCenter(sdkKey); | ||
NotificationCenter notificationCenter2 = NotificationRegistry.getInternalNotificationCenter(sdkKey); | ||
assertEquals(notificationCenter1, notificationCenter2); | ||
} | ||
|
||
@Test | ||
public void getSameNotificationcenterWhenSDKkeyIsSameButNotNull() { | ||
String sdkKey = "testSDkKey"; | ||
NotificationCenter notificationCenter1 = NotificationRegistry.getInternalNotificationCenter(sdkKey); | ||
NotificationCenter notificationCenter2 = NotificationRegistry.getInternalNotificationCenter(sdkKey); | ||
assertEquals(notificationCenter1, notificationCenter2); | ||
} | ||
|
||
@SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION") | ||
@Test | ||
public void getSameNotificationcenterWhenSDKkeyIsNullAndAnotherIsEmpty() { | ||
String sdkKey1 = ""; | ||
String sdkKey2 = null; | ||
NotificationCenter notificationCenter1 = NotificationRegistry.getInternalNotificationCenter(sdkKey1); | ||
NotificationCenter notificationCenter2 = NotificationRegistry.getInternalNotificationCenter(sdkKey2); | ||
assertEquals(notificationCenter1, notificationCenter2); | ||
} | ||
|
||
@Test | ||
public void getDifferentNotificationcenterWhenSDKkeyIsNotSame() { | ||
String sdkKey1 = "testSDkKey1"; | ||
String sdkKey2 = "testSDkKey2"; | ||
NotificationCenter notificationCenter1 = NotificationRegistry.getInternalNotificationCenter(sdkKey1); | ||
NotificationCenter notificationCenter2 = NotificationRegistry.getInternalNotificationCenter(sdkKey2); | ||
Assert.assertNotEquals(notificationCenter1, notificationCenter2); | ||
} | ||
|
||
@Test | ||
public void clearRegistryNotificationcenterClearsOldNotificationCenter() { | ||
String sdkKey1 = "testSDkKey1"; | ||
NotificationCenter notificationCenter1 = NotificationRegistry.getInternalNotificationCenter(sdkKey1); | ||
NotificationRegistry.clearNotificationCenterRegistry(); | ||
NotificationCenter notificationCenter2 = NotificationRegistry.getInternalNotificationCenter(sdkKey1); | ||
|
||
Assert.assertNotEquals(notificationCenter1, notificationCenter2); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,7 @@ public class HttpProjectConfigManager extends PollingProjectConfigManager { | |
private final URI uri; | ||
private final String datafileAccessToken; | ||
private String datafileLastModified; | ||
private String sdkKey; | ||
|
||
private HttpProjectConfigManager(long period, | ||
TimeUnit timeUnit, | ||
|
@@ -73,11 +74,13 @@ private HttpProjectConfigManager(long period, | |
String datafileAccessToken, | ||
long blockingTimeoutPeriod, | ||
TimeUnit blockingTimeoutUnit, | ||
NotificationCenter notificationCenter) { | ||
NotificationCenter notificationCenter, | ||
String sdkKey) { | ||
|
||
super(period, timeUnit, blockingTimeoutPeriod, blockingTimeoutUnit, notificationCenter); | ||
this.httpClient = httpClient; | ||
this.uri = URI.create(url); | ||
this.datafileAccessToken = datafileAccessToken; | ||
this.sdkKey = sdkKey; | ||
} | ||
|
||
public URI getUri() { | ||
|
@@ -167,6 +170,11 @@ public static Builder builder() { | |
return new Builder(); | ||
} | ||
|
||
@Override | ||
public String getSDKKey() { | ||
return this.sdkKey; | ||
} | ||
|
||
public static class Builder { | ||
private String datafile; | ||
private String url; | ||
|
@@ -357,7 +365,8 @@ public HttpProjectConfigManager build(boolean defer) { | |
datafileAccessToken, | ||
blockingTimeoutPeriod, | ||
blockingTimeoutUnit, | ||
notificationCenter); | ||
notificationCenter, | ||
sdkKey); | ||
mnoman09 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
if (datafile != null) { | ||
try { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.