Skip to content

feat(datafile-cache): Add support for datafile cache service. #340

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

feat(datafile-cache): Add support for datafile cache service. #340

wants to merge 4 commits into from

Conversation

msohailhussain
Copy link
Contributor

@msohailhussain msohailhussain commented Apr 4, 2022 •
edited by yasirfolio3
Loading

Summary

  • Add support for datafile cache service which allows nodes in a cluster to re-use cached datafile from redis instance.
  • Fix context for user profile service.

Tests

  • All Unit and FSC tests should pass.

Redis Datafile Cache Service

To enable redis datafile cache service, Update the config.yaml file with your datafileCacheService config as shown below:

## Datafile cache to save initial datafile fetch call in a multi-node environment.
datafileCacheService:
      enabled: true
      services:
        redis: 
          host: "your_redis_host_address"
          password: "your_password"
          database: 0

msohailhussain
msohailhussain commented Apr 4, 2022
View reviewed changes
cmd/optimizely/testdata/default.yaml
@@ -32,6 +32,10 @@ client:
datafileURLTemplate: "https://localhost/v1/%s.json"
eventURL: "https://logx.localhost.com/v1"
sdkKeyRegex: "custom-regex"
datafileCacheService:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

datafileCacheServices will be more relevant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to keep it similar to the naming convention we are following for userProfileService.

msohailhussain
msohailhussain commented Apr 4, 2022
View reviewed changes
cmd/optimizely/testdata/default.yaml
@@ -32,6 +32,10 @@ client:
datafileURLTemplate: "https://localhost/v1/%s.json"
eventURL: "https://logx.localhost.com/v1"
sdkKeyRegex: "custom-regex"
datafileCacheService:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add one more property enableDatafileCacheService and based on that, execute the logic.

msohailhussain
msohailhussain commented Apr 4, 2022
View reviewed changes
pkg/optimizely/cache.go
options := []sdkconfig.OptionFunc{}

// Check if datafile is already present in cache
cachedDatafile, cacheService := getDatafileFromCacheService(ctx, sdkKey, conf)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a condition of cache is enabled then go into this method.

msohailhussain
msohailhussain commented Apr 4, 2022
View reviewed changes
pkg/optimizely/cache.go
@@ -226,7 +249,7 @@ func defaultLoader(
}

var clientUserProfileService decision.UserProfileService
if clientUserProfileService = getUserProfileService(sdkKey, userProfileServiceMap, conf); clientUserProfileService != nil {
if clientUserProfileService = getUserProfileService(ctx, sdkKey, userProfileServiceMap, conf); clientUserProfileService != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need to change this method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier, We were giving UPS a different context altogether which might've resulted in UPS requests still running for a little while after agent had shut down. This fix is to pass the original agent context to UPS so that once agents shuts down, UPS terminates all its requests as well.

msohailhussain
msohailhussain commented Apr 4, 2022
View reviewed changes
pkg/optimizely/datafilecacheservice/redis_cache_service.go
}

// RedisCacheService represents the redis implementation of DatafileCacheService interface
type RedisCacheService struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this struct is same for UPS as well, can't we use the same.

@yasirfolio3 yasirfolio3 changed the title DRAFT - Yasir/datafile cache feat(datafile-cache): Add support for datafile cache service. Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yasirfolio3 yasirfolio3 yasirfolio3 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@msohailhussain @yasirfolio3