-
Notifications
You must be signed in to change notification settings - Fork 26
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
[FSSDK-9841] adds cache to ups lookup call #406
base: master
Are you sure you want to change the base?
Conversation
Hi @204Constie |
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.
Shared some thoughts.
|
||
"github.com/optimizely/agent/plugins/userprofileservice" | ||
"github.com/optimizely/go-sdk/pkg/decision" | ||
"github.com/optimizely/go-sdk/pkg/logging" | ||
"github.com/optimizely/go-sdk/pkg/utils" | ||
"github.com/patrickmn/go-cache" |
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.
I have found this: "go-cache is an in-memory key:value store/cache similar to memcached that is suitable for applications running on a single machine.".
When the agent will be run with multiple replicas, this might not help. So, I think, this needs to be configurable. And by default, the caching should be disabled. Users have to manually enable it according to the underlying system. WDYT?
@@ -70,7 +77,9 @@ func (r *RestUserProfileService) Lookup(userID string) (profile decision.UserPro | |||
return | |||
} | |||
|
|||
return convertToUserProfile(userProfileMap, userIDKey) | |||
userProfile := convertToUserProfile(userProfileMap, userIDKey) | |||
r.Cache.Set(userProfile.ID, userProfile, cache.DefaultExpiration) |
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.
Expiration time may vary user to user. So, it can be configurable.
@@ -162,10 +171,12 @@ func (r *RestUserProfileService) performRequest(requestURL, method string, param | |||
} | |||
|
|||
func init() { | |||
c := cache.New(5*time.Minute, 10*time.Minute) |
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.
Can we use constants with a relevant name for those time values?
Summary
the service has very poor performance when one request is done for multiple experiments at once
the PR adds caching on lookup method to prevent async request loop when not necessary
Issues