[FSSDK-8838] feat(odp-cache): Adds support for odp cache.#365

yasirfolio3 · 2023-03-03T14:28:48Z

Summary

Provides 2 out of the box ODP cache including in-memory and redis.
in-memory will use the builtin LRU cache with user provided size and timeout.
Adds support for custom odp cache.
If no odp cache is provided, agent will use go-sdk's in built LRU Cache with default size and timeout.

Out of Box ODP Cache Usage

To use the in-memory ODPCache, update the config.yaml as shown below:

## configure optional ODP Cache odp: segmentsCache: default: "in-memory" services: in-memory: ## 0 means cache will be disabled size: 0 ## timeout after which the cached item will become invalid. ## 0 means records will never be deleted timeout: 0s

To use the redis ODPCache, update the config.yaml as shown below:

## configure optional ODP Cache odp: segmentsCache: default: "redis" services: redis: host: "your_host" password: "your_password" database: 0 ## your database timeout: 0s

Custom ODPCache Implementation

To implement a custom odp cache, followings steps need to be taken:

Create a struct that implements the cache.Cache interface in plugins/odpcache/services.
Add a init method inside your ODPCache file as shown below:

func init(){myCacheCreator := func() cache.Cache{return &yourCacheStruct{} } odpcache.Add("my_cache_name", myCacheCreator) }

Update the config.yaml file with your ODPCache config as shown below:

## configure optional ODPCache odp segmentsCache: default: "my_cache_name" services: my_cache_name: ## Add those parameters here that need to be mapped to the ODPCache ## For example, if the ODP struct has a json mappable property called `host` ## it can updated with value `abc.com` as shown host: “abc.com”

If a user has created multiple ODPCache services and wants to override the defaultODPCache for a specific sdkKey, they can do so by providing the ODPCache name in the request Header X-Optimizely-ODP-Cache-Name.
Whenever a request is made with a unique sdkKey, The agent node handling that request creates and caches a new ODPCache. To keep the ODPCache type consistent among all nodes in a cluster, it is recommended to send the request Header X-Optimizely-ODP-Cache-Name in every request made.

Tests

Unit tests added.
FSC should pass.

Ticket

FSSDK-8838

* Adds support for odp options in config.yaml

jaeopt

Looks great! Some suggestions to reuse go-sdk cache. Let's discuss options.

jaeopt · 2023-03-07T23:16:49Z

config/config.go

+EventsRequestTimeout time.Duration`json:"eventsRequestTimeout"`
+EventsFlushInterval time.Duration`json:"eventsFlushInterval"`
+Disablebool`json:"disable"`


Suggested change
EventsRequestTimeouttime.Duration`json:"eventsRequestTimeout"`
EventsFlushIntervaltime.Duration`json:"eventsFlushInterval"`
Disablebool`json:"disable"`
Disablebool`json:"disable"`
EventsRequestTimeouttime.Duration`json:"eventsRequestTimeout"`
EventsFlushIntervaltime.Duration`json:"eventsFlushInterval"`

jaeopt · 2023-03-07T23:23:24Z

plugins/odpcache/README.md

@@ -0,0 +1,64 @@
+# ODP Cache
+Use a ODP Cache to persist segments and ensure they are sticky. 


Suggested change
Use a ODP Cache to persist segments and ensure they are sticky.
Use a ODP Cache to cache user segments fetched from the ODP server.

jaeopt · 2023-03-07T23:33:18Z

plugins/odpcache/services/in_memory_cache.go

+)
+
+// InMemoryCache represents the in-memory implementation of Cache interface
+typeInMemoryCachestruct{


Not clear about the value of providing an option to use this in-memory cache. It looks like instantiating the same URLCache implemented in go-sdk. Can't we use the default one already in go-sdk?

We are now using this as default odp cache with both timeout and size as arguments. Following is the default configuration:
odp: ## Disable odp disable: false ## Timeout in seconds after which event requests will timeout. eventsRequestTimeout: 10s ## Flush interval in seconds for odp events eventsFlushInterval: 1s ## Timeout in seconds after which segment requests will timeout. segmentsRequestTimeout: 10s ## This will override all other settings segmentsCache: default: "in-memory" services: in-memory: size: 10000 timeout: 600 # seconds # redis: # host: "localhost:6379" # password: "" # database: 0 # timeout: 0 # seconds

jaeopt · 2023-03-09T18:54:07Z

cmd/optimizely/main.go

 }

+// Check if JSON string was set using OPTIMIZELY_CLIENT_ODP_SEGMENTSCACHE environment variable
+ifodpSegmentsCache:=v.GetStringMap("client.odp.segmentsCache"); odpSegmentsCache!=nil{


Wondering why we need this support for odpCache only? What about other odp config like "client.odp"?

No specific reason, its just something that we did for ups aswell.

jaeopt · 2023-03-09T18:59:52Z

config.yaml

+segmentsCacheTimeout: 600s
+## Timeout in seconds after which segment requests will timeout.
+segmentsRequestTimeout: 10s
+## This will override all other settings


Can we be more specific about overriding? CacheSize and CacheTimeout will be overriden. Others like "disable" etc?

deleted this comment as segmentsCacheTimeout and segmentsCacheSize no longer exist outside of the scope of in-memory cache.

jaeopt · 2023-03-09T19:01:17Z

config.yaml

+segmentsRequestTimeout: 10s
+## This will override all other settings
+segmentsCache:
+default: ""


What does this empty default imply? Will agent use go-sdk native URLCache with segmentCacheSize and segmentsCacheTimeout? Or is it cache disabled?

as per the new implementation, empty default simply means that go-sdk will use its default LRU cache with default timeout and size

jaeopt · 2023-03-09T19:59:10Z

config.yaml

+# in-memory: 
+# size: 0
+# timeout: 0 # seconds


Is this in-memory cache the same as one in go-sdk (implemented and active by default)?
If so, we can use size and timeout for its config (segmentsCacheSize and segmentsCacheTimeout above can be removed).
What about "in-memory" as default for "default: in-memory"? So, they'll use go-sdk cache unless override with redis etc.

Does this default config make sense?
disable: false eventsRequestTimeout: 10s eventsFlushInterval: 1s segmentsRequestTimeout: 10s segmentsCache: default: "in-memory" services: in-memory: size: 10000 timeout: 600s # redis: # host: #

thanks for the suggestion, went with the same approach.

jaeopt · 2023-03-09T22:27:28Z

config.yaml

+# size: 0
+# timeout: 0 # seconds
+# redis: 
+# host: "localhost:6379"


Don't we need to control redis timeout as well? We can reuse segmentsCacheTimeout but it may be confusing since we do not use segmentsCacheSize for redis.

added timeout to redis aswell.

jaeopt · 2023-03-09T22:28:12Z

plugins/odpcache/services/redis_cache.go

+funcinit(){
+redisCacheCreator:=func() cache.Cache{
+return&RedisCache{
+Expiration: 0*time.Second,


We need timeout control for redis cache. Can't we add timeout to config?

jaeopt

All changes look good! A few small fixes suggested.

jaeopt · 2023-03-13T22:22:27Z

cmd/optimizely/testdata/default.yaml

 redis: 
 host: "localhost:6379"
 password: ""
+timeout: 5


5s was possible if the key was at the root and viper was unmarshalling it. In this case we are unmarshalling it at runtime using native json library which does not support converting 5s to time duration.

jaeopt · 2023-03-13T22:23:15Z

config.yaml

-#timeout: 0 # seconds
+in-memory: 
+size: 10000
+timeout: 600# seconds


same as above.

jaeopt · 2023-03-13T22:23:58Z

config.yaml

 # host: "localhost:6379"
 # password: ""
 # database: 0
+# timeout: 0 # seconds


600s default?

same as above.

jaeopt · 2023-03-13T22:28:16Z

config.yaml

-segmentsCacheTimeout: 600s
 ## Timeout in seconds after which segment requests will timeout.
 segmentsRequestTimeout: 10s
 ## This will override all other settings


No overriding any more. This line should be removed.

jaeopt

Looks great! A few minor fixes suggested.

jaeopt · 2023-03-17T17:16:28Z

plugins/odpcache/README.md

+ in-memory: 
+ ## 0 means cache will be disabled
+ size: 0
+ ## timeout after which least recently used records will be deleted


Suggested change
## timeout after which least recently used records will be deleted
## timeout after which the cached item will become invalid.

jaeopt · 2023-03-17T17:18:23Z

plugins/odpcache/README.md

+## configure optional ODP Cache
+odp:
+ segmentsCache:
+ default: "in-memory"


It can be better if default "default" is clarified. If no segmentsCache is defined (or no default is defined), we will use the default in-memory with default size and timeout?

This statement is true, if none provided then sdk uses its own in-memory with default values: https://github.com/optimizely/go-sdk/blob/master/pkg/odp/segment/segment_manager.go#L87

jaeopt · 2023-03-17T17:35:41Z

plugins/utils/duration.go

+
+switchvalue:=unmarshalledJSON.(type){
+casefloat64:
+duration.Duration=time.Duration(value)


Do we accept number without unit? What is the default unit for this case?

yes we do, default unit is nanosecond by golang.

jaeopt

LGTM!

yasirfolio3 added 13 commits February 9, 2023 15:29

Initial commit.
575e74c

Adding in memory and redis cache plugins.
388bff7

Adding tests for UPS Redis cache.
b6c07a1

WIP.
2417c14

Add readme
3913111

Implementations
4186605

Merge branch 'master' into yasir/odp-cache
a285088

fixing unit test.
bf7259a

headers updated.
4565d21

nit fixes.
24ff939

updating go.mod
7041091

fixes to pass hadolint tests.
0f0e681

Merge branch 'master' into yasir/odp-cache
4e7ede9

yasirfolio3 changed the title ~~feat(odp-cache): Adds support for odp cache.~~[FSSDK-8838] feat(odp-cache): Adds support for odp cache.Mar 3, 2023

yasirfolio3and others added 6 commits March 3, 2023 14:09

nits fixed.
ad2c25f

nits fixed.
5b53595

Updated config.yaml
fa4185c

FSSDK-8970: Adds support for odp options in config. (#366)
093261c
* Adds support for odp options in config.yaml

Suggested changes made.
44ab65c

Merge branch 'master' into yasir/odp-cache
1fc7b05

yasirfolio3 requested a review from jaeopt March 7, 2023 20:43

jaeopt requested changes Mar 9, 2023
View reviewed changes

yasirfolio3and others added 2 commits March 13, 2023 10:30

Merge branch 'master' into yasir/odp-cache
bffb21a

suggested changes made.
8abfda3

yasirfolio3 requested a review from jaeopt March 13, 2023 19:08

jaeopt requested changes Mar 13, 2023
View reviewed changes

yasirfolio3 added 2 commits March 14, 2023 10:10

nit fixes.
344c849

Merge branch 'master' into yasir/odp-cache
3cc8c71

yasirfolio3 requested a review from jaeopt March 14, 2023 15:49

Merge branch 'master' into yasir/odp-cache
ff00d6f

yasirfolio3and others added 2 commits March 17, 2023 13:00

Added support for time units in config.yaml
7cb5133

Merge branch 'master' into yasir/odp-cache
9a49dc1

jaeopt requested changes Mar 17, 2023
View reviewed changes

suggested changes made.
8a2cce0

yasirfolio3 requested a review from jaeopt March 17, 2023 18:36

jaeopt approved these changes Mar 17, 2023
View reviewed changes

yasirfolio3 merged commit ce3b3a7 into masterMar 17, 2023

yasirfolio3 deleted the yasir/odp-cache branch March 17, 2023 20:49

-EventsRequestTimeouttime.Duration`json:"eventsRequestTimeout"`
-EventsFlushIntervaltime.Duration`json:"eventsFlushInterval"`
-Disablebool`json:"disable"`
+Disablebool`json:"disable"`
+EventsRequestTimeouttime.Duration`json:"eventsRequestTimeout"`
+EventsFlushIntervaltime.Duration`json:"eventsFlushInterval"`

		@@ -0,0 +1,64 @@
		# ODP Cache
		Use a ODP Cache to persist segments and ensure they are sticky.

	Use a ODP Cache to persist segments and ensure they are sticky.
	Use a ODP Cache to cache user segments fetched from the ODP server.

	## timeout after which least recently used records will be deleted
	## timeout after which the cached item will become invalid.

[FSSDK-8838] feat(odp-cache): Adds support for odp cache.#365

[FSSDK-8838] feat(odp-cache): Adds support for odp cache. #365

Uh oh!

Conversation

yasirfolio3 commented Mar 3, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Out of Box ODP Cache Usage

Custom ODPCache Implementation

Tests

Ticket

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!

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!

yasirfolio3Mar 13, 2023• 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!

jaeoptMar 9, 2023• 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!

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!

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!

jaeoptMar 17, 2023• 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

yasirfolio3 commented Mar 3, 2023•
edited
Loading

yasirfolio3Mar 13, 2023•
edited
Loading

jaeoptMar 9, 2023•
edited
Loading

jaeoptMar 17, 2023•
edited
Loading