feat: Added ODPSegmentManager

[mikechu-optimizely]

Summary

Added an ODPSegmentManager which provides functionality to fetch segments from ODP server and cache and re-use them.

Test plan

• Manually tested thoroughly
• Added new unit tests
• Existing unit tests pass
• All Full-stack compatibility suite tests pass

Issues

FSSDK-8408

[msohailhussain]

lgtm. 2 nits please address.

[msohailhussain]

don't return inside the lock statement. get a result in a variable inside lock and return outside of lock.

[mikechu-optimizely]

Great advice. Thanks for the learning.

[msohailhussain]

Can you please check with jae, Returning empty list may be revised.

[mikechu-optimizely]

Will do. @jaeopt ☝️

[mikechu-optimizely]

I'm not completely sure how to read it, but it looks like the Swift implementation returns null

Java returns empty collection.

Python looks like it returns an empty array.

Go also seems to return an empty slice.

---

I do see I need a correction when ODP config is not ready. I should have returned null instead.

[jaeopt]

@mikechu-optimizely For any fetch error (odp not integrated, network errors, etc), we should return null. For empty segmentToCheck, we should return empty array.

[jaeopt]

Looks good. A few changes suggested.

[jaeopt]

Isn't this too slow O(n) for every item search when cache is full? Should be O(1). Wondering why your original "OrderedSet"-based one was discarded.

[mikechu-optimizely]

Thanks for asking this question. It got me thinking more critically and then researching.

---

The previous version is LINQ syntactic sugar on top of this new chained version to satisfy the linter. 🤷

---

Regarding time complexity, as I understood/understand it, you're right the .Where() would be O(N). The index .FirstOrDefault() should allow looping over the collection and stop on the first matching case, giving us O(1) [correct?]

The .Select() will return the matching Key which is what's needed to remove the item from the cache.

cc @msohailhussain

[mikechu-optimizely]

@jaeopt Thinking more on this: LINQ does have its own overhead.

Should I change this to a standard loop?

[jaeopt]

@mikechu-optimizely the current looping should be fixed since it'll slow down the replacement search significantly.
Wondering why OrderedSet solution was discarded?
If it can't be used, we can also consider keeping key in the ItemWrapper. See https://github.com/optimizely/swift-sdk/blob/86a328f942da7955873727247ccd675d6b246f80/Sources/ODP/LruCache.swift#L84

[mikechu-optimizely]

@jaeopt I've added the Key as you suggested to ItemWrapper. This allowed me to remove the loop/LINQ. 👍

I'm not unsure I see the OrderedSet. Do you mean these lines that I'm replacing?

 var leastRecentlyUsedItemKey = (from cacheItem in _cache
    where cacheItem.Value == leastRecentlyUsedItem.Value
    select cacheItem.Key).FirstOrDefault();

if so, this LINQ expression is not different from the method chaining version. Happy to discuss on the scheduled meeting I sent for later today so we can close up this PR.

I like the newest solution and hope it meets your standard. If you like the submission and Approve, we can cancel the meeting if you'd like.

[jaeopt]

@mikechu-optimizely I saw your origin LRUCache PR uses private readonly OrderedDictionary _orderedDictionary = new OrderedDictionary();. It looked like a good solution and wondering why it was discarded.

[mikechu-optimizely]

Ohhh. I understand. Thank you for clarifying and digging into the history.

It's likely that I was following the Java implementation before it was updated to use LinkedHashMap.

OrderedDictionary is a better solution. I've updated.

Please review the latest commit.

[jaeopt]

@mikechu-optimizely For any fetch error (odp not integrated, network errors, etc), we should return null. For empty segmentToCheck, we should return empty array.

[jaeopt]

LGTM