feat: Added ODPManager implementation by zashraf1985 · Pull Request #489 · optimizely/java-sdk

zashraf1985 · 2022-10-14T08:07:50Z

Summary

Added ODPManager Implementation which does the following.

Initializes and provides access to ODPEventManager and ODPSegmentManager
Provides updated ODPConfig settings to event manager and segment manager.
Stops Event Manager thread when closed.

Test plan

Manually tested thoroughly
Added unit tests.

Issues

FSSDK-8388

jaeopt

Looks good. Some changes and clarifications suggested.

jaeopt · 2022-10-14T22:36:03Z

core-api/src/main/java/com/optimizely/ab/odp/ODPManager.java

+        this(odpConfig, new ODPSegmentManager(odpConfig, apiManager), new ODPEventManager(odpConfig, apiManager));
+    }
+
+    public ODPManager(@Nonnull ODPConfig odpConfig, @Nonnull ODPSegmentManager segmentManager, @Nonnull ODPEventManager eventManager) {


Customization of segmentCache size/timeout and segmentCache itself as well?

We also need to support for "disable" entire ODP functions.

There are two constructors.

First one only takes the two mandatory arguments of ODPConfig and ApiManager and initializes everything else on default values.

If you want to customize anything at all, use the second constructor which expects you to pass everything from the outside. This is where you will have a chance to initialize your own SegmentManager with your own cacheOptions.

I will recommend to keep these two options for now. We can provide other combinations at the level of factory methods to create the SDK instance later as needed. What do you think?

I am thinking of handling disable / enable at optimizely level. When enabled, ODP object will be there and it will always work. When disabled, there will be no ODP instance at all. What do you suggest?

There are two constructors.

First one only takes the two mandatory arguments of ODPConfig and ApiManager and initializes everything else on default values.

If you want to customize anything at all, use the second constructor which expects you to pass everything from the outside. This is where you will have a chance to initialize your own SegmentManager with your own cacheOptions.

I will recommend to keep these two options for now. We can provide other combinations at the level of factory methods to create the SDK instance later as needed. What do you think?

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.

I am thinking of handling disable / enable at optimizely level. When enabled, ODP object will be there and it will always work. When disabled, there will be no ODP instance at all. What do you suggest?

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.

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?

jaeopt · 2022-10-14T22:45:11Z

core-api/src/main/java/com/optimizely/ab/odp/ODPManager.java

+    public void close() {
+        eventManager.stop();
+    }
+}


We do not have top-level fetchSegments and sendEvent apis. Is it a plan to open directly from OdpEventManager and OdpSegmentManager?

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?)

Disable: I am expecting disable to be handled in optimizely object which will never go to ODPManager at all if disabled.

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 eventManager itself. What do you suggest?

See TDD which defines accepted types for event data.

about top level fetch and event methods, SegmentManager and EventManager provide some overrides. If i add all those overrides, it looks redundant. I thought it made more sense to provide access to segmentManager and eventManager directly through getters to use those methods.

jaeopt · 2022-10-14T22:46:53Z

core-api/src/main/java/com/optimizely/ab/odp/ODPManager.java

+    }
+
+    public Boolean updateSettings(String apiHost, String apiKey, List<String> allSegments) {
+        ODPConfig newConfig = new ODPConfig(apiKey, apiHost, allSegments);


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.

jaeopt

Looks good. A couple of small changes suggested.

jaeopt · 2022-10-19T17:08:24Z

core-api/src/main/java/com/optimizely/ab/odp/ODPEvent.java

+
+    @Transient
+    public Boolean isDataValid() {
+        for (Object entry: this.data.values()) {


null is also valid

jaeopt · 2022-10-19T17:27:06Z

core-api/src/main/java/com/optimizely/ab/odp/ODPEventManager.java


    public void sendEvent(ODPEvent event) {
+        if (!event.isDataValid()) {
+            logger.error("Error Sending Event: ODP data is not valid");


Can we change it to standard message - "ODP event send failed ()"

2. Fixed ODP event error message

jaeopt

LGTM

Implemented ODP Manager and added unit tests

1ab0e1a

zashraf1985 requested a review from a team as a code owner October 14, 2022 08:07

optimizely-codereview-bot assigned zashraf1985 Oct 14, 2022

zashraf1985 added 4 commits October 14, 2022 01:13

increased wait time for event to flush

007be89

Added license headers.

f493cbf

fixed SpotBugsTest

cf08d59

Added a line break at the end of Test file.

ba0365f

zashraf1985 removed their assignment Oct 14, 2022

zashraf1985 requested a review from jaeopt October 14, 2022 16:36

optimizely-codereview-bot added the 🤖 4 hrs label Oct 14, 2022

jaeopt requested changes Oct 14, 2022

View reviewed changes

optimizely-codereview-bot removed the 🤖 4 hrs label Oct 14, 2022

optimizely-codereview-bot assigned zashraf1985 Oct 14, 2022

zashraf1985 added 2 commits October 17, 2022 13:13

Added force flush on when settings update.

ad5c86a

Added data types verifcation for events

3bf6a03

zashraf1985 removed their assignment Oct 18, 2022

optimizely-codereview-bot requested a review from jaeopt October 18, 2022 18:27

jaeopt requested changes Oct 19, 2022

View reviewed changes

optimizely-codereview-bot assigned zashraf1985 Oct 19, 2022

1. Allowed null value in event data

620ca8d

2. Fixed ODP event error message

zashraf1985 removed their assignment Oct 20, 2022

optimizely-codereview-bot requested a review from jaeopt October 20, 2022 01:42

jaeopt approved these changes Oct 20, 2022

View reviewed changes

zashraf1985 merged commit 7428c4c into master Oct 20, 2022

zashraf1985 deleted the zeeshan/ats-odp-manager branch October 20, 2022 18:31

Conversation

zashraf1985 commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Issues

Uh oh!

jaeopt 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!

jaeopt Oct 17, 2022 • 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!

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!

jaeopt 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!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zashraf1985 commented Oct 14, 2022 •

edited

Loading

jaeopt Oct 17, 2022 •

edited

Loading