-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: Added ODPManager implementation #489
Changes from all commits
1ab0e1a
007be89
f493cbf
cf08d59
ba0365f
ad5c86a
3bf6a03
620ca8d
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 |
---|---|---|
@@ -0,0 +1,61 @@ | ||
/** | ||
* | ||
* Copyright 2022, 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.odp; | ||
|
||
import javax.annotation.Nonnull; | ||
import java.util.List; | ||
|
||
public class ODPManager { | ||
private volatile ODPConfig odpConfig; | ||
private final ODPSegmentManager segmentManager; | ||
private final ODPEventManager eventManager; | ||
|
||
public ODPManager(@Nonnull ODPConfig odpConfig, @Nonnull ODPApiManager apiManager) { | ||
this(odpConfig, new ODPSegmentManager(odpConfig, apiManager), new ODPEventManager(odpConfig, apiManager)); | ||
} | ||
|
||
public ODPManager(@Nonnull ODPConfig odpConfig, @Nonnull ODPSegmentManager segmentManager, @Nonnull ODPEventManager eventManager) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Customization of segmentCache size/timeout and segmentCache itself as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We also need to support for "disable" entire ODP functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two constructors.
I will recommend to keep these two options for now. We can provide other combinations at the level of factory methods to create the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am thinking of handling disable / enable at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see adjusting cache size and timeout more likely options for clients rather than full Cache and OdpManager customazations. Providing that path with factory will be also a good solution too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I was concerned about "if odpManager == null" checking in many places we need odpManager. We can try wrap those checking in OdpManager. Other than that, disabling at the top level is also a good option too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would advise to defer this discussion till the top level PR. We can then see if it looks good at top level other wise i will add this to ODPManager. what do you say? |
||
this.odpConfig = odpConfig; | ||
this.segmentManager = segmentManager; | ||
this.eventManager = eventManager; | ||
this.eventManager.start(); | ||
} | ||
|
||
public ODPSegmentManager getSegmentManager() { | ||
return segmentManager; | ||
} | ||
|
||
public ODPEventManager getEventManager() { | ||
return eventManager; | ||
} | ||
|
||
public Boolean updateSettings(String apiHost, String apiKey, List<String> allSegments) { | ||
ODPConfig newConfig = new ODPConfig(apiKey, apiHost, allSegments); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we flush events in queue before updating apiKey/apiHost? It should not be common but when clients change the ODP account, all events in the queue should be flushed to the old account. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
if (!odpConfig.equals(newConfig)) { | ||
odpConfig = newConfig; | ||
eventManager.updateSettings(odpConfig); | ||
segmentManager.resetCache(); | ||
segmentManager.updateSettings(odpConfig); | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
public void close() { | ||
eventManager.stop(); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not have top-level There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is expected to filter out api requests in this level (like "disable" discards all requests). Also sendEvent data type should be validated here (are we doing this in OdpEventManager?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disable: I am expecting disable to be handled in SendEvent Data validation: I am not sure what validation needs to be done. But if there is one, i would like to add that to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See TDD which defines accepted types for event data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. about top level fetch and event methods, |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,123 @@ | ||
/** | ||
* | ||
* Copyright 2022, 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.odp; | ||
|
||
import org.junit.Before; | ||
import org.junit.Test; | ||
import org.mockito.Mock; | ||
import org.mockito.Mockito; | ||
|
||
import java.util.Arrays; | ||
|
||
import static org.mockito.Matchers.*; | ||
import static org.mockito.Mockito.*; | ||
import static org.junit.Assert.*; | ||
|
||
public class ODPManagerTest { | ||
private static final String API_RESPONSE = "{\"data\":{\"customer\":{\"audiences\":{\"edges\":[{\"node\":{\"name\":\"segment1\",\"state\":\"qualified\"}},{\"node\":{\"name\":\"segment2\",\"state\":\"qualified\"}}]}}}}"; | ||
|
||
@Mock | ||
ODPApiManager mockApiManager; | ||
|
||
@Mock | ||
ODPEventManager mockEventManager; | ||
|
||
@Mock | ||
ODPSegmentManager mockSegmentManager; | ||
|
||
@Before | ||
public void setup() { | ||
mockApiManager = mock(ODPApiManager.class); | ||
mockEventManager = mock(ODPEventManager.class); | ||
mockSegmentManager = mock(ODPSegmentManager.class); | ||
} | ||
|
||
@Test | ||
public void shouldStartEventManagerWhenODPManagerIsInitialized() { | ||
ODPConfig config = new ODPConfig("test-key", "test-host"); | ||
new ODPManager(config, mockSegmentManager, mockEventManager); | ||
verify(mockEventManager, times(1)).start(); | ||
} | ||
|
||
@Test | ||
public void shouldStopEventManagerWhenCloseIsCalled() { | ||
ODPConfig config = new ODPConfig("test-key", "test-host"); | ||
ODPManager odpManager = new ODPManager(config, mockSegmentManager, mockEventManager); | ||
|
||
// Stop is not called in the default flow. | ||
verify(mockEventManager, times(0)).stop(); | ||
|
||
odpManager.close(); | ||
// stop should be called when odpManager is closed. | ||
verify(mockEventManager, times(1)).stop(); | ||
} | ||
|
||
@Test | ||
public void shouldUseNewSettingsInEventManagerWhenODPConfigIsUpdated() throws InterruptedException { | ||
Mockito.when(mockApiManager.sendEvents(any(), any(), any())).thenReturn(200); | ||
ODPConfig config = new ODPConfig("test-key", "test-host", Arrays.asList("segment1", "segment2")); | ||
ODPManager odpManager = new ODPManager(config, mockApiManager); | ||
|
||
odpManager.getEventManager().identifyUser("vuid", "fsuid"); | ||
Thread.sleep(2000); | ||
verify(mockApiManager, times(1)) | ||
.sendEvents(eq("test-key"), eq("test-host/v3/events"), any()); | ||
|
||
odpManager.updateSettings("test-host-updated", "test-key-updated", Arrays.asList("segment1")); | ||
odpManager.getEventManager().identifyUser("vuid", "fsuid"); | ||
Thread.sleep(1200); | ||
verify(mockApiManager, times(1)) | ||
.sendEvents(eq("test-key-updated"), eq("test-host-updated/v3/events"), any()); | ||
} | ||
|
||
@Test | ||
public void shouldUseNewSettingsInSegmentManagerWhenODPConfigIsUpdated() { | ||
Mockito.when(mockApiManager.fetchQualifiedSegments(anyString(), anyString(), anyString(), anyString(), anyList())) | ||
.thenReturn(API_RESPONSE); | ||
ODPConfig config = new ODPConfig("test-key", "test-host", Arrays.asList("segment1", "segment2")); | ||
ODPManager odpManager = new ODPManager(config, mockApiManager); | ||
|
||
odpManager.getSegmentManager().getQualifiedSegments("test-id"); | ||
verify(mockApiManager, times(1)) | ||
.fetchQualifiedSegments(eq("test-key"), eq("test-host/v3/graphql"), any(), any(), any()); | ||
|
||
odpManager.updateSettings("test-host-updated", "test-key-updated", Arrays.asList("segment1")); | ||
odpManager.getSegmentManager().getQualifiedSegments("test-id"); | ||
verify(mockApiManager, times(1)) | ||
.fetchQualifiedSegments(eq("test-key-updated"), eq("test-host-updated/v3/graphql"), any(), any(), any()); | ||
} | ||
|
||
@Test | ||
public void shouldGetEventManager() { | ||
ODPConfig config = new ODPConfig("test-key", "test-host"); | ||
ODPManager odpManager = new ODPManager(config, mockSegmentManager, mockEventManager); | ||
assertNotNull(odpManager.getEventManager()); | ||
|
||
odpManager = new ODPManager(config, mockApiManager); | ||
assertNotNull(odpManager.getEventManager()); | ||
} | ||
|
||
@Test | ||
public void shouldGetSegmentManager() { | ||
ODPConfig config = new ODPConfig("test-key", "test-host"); | ||
ODPManager odpManager = new ODPManager(config, mockSegmentManager, mockEventManager); | ||
assertNotNull(odpManager.getSegmentManager()); | ||
|
||
odpManager = new ODPManager(config, mockApiManager); | ||
assertNotNull(odpManager.getSegmentManager()); | ||
} | ||
} |
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.
null is also valid
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.
Done