Skip to content

feat: Added caching for OptimizelyConfig object #352

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

Merged
merged 30 commits into from
Jan 9, 2020

Conversation

fayyazarshad
Copy link
Contributor

@fayyazarshad fayyazarshad commented Dec 20, 2019

Summary

Updated the OptimizelyConfig implementation to always return cached object. OptimizelyConfig object will only be updated only when new ProjectConfig is received.

Test Plan

Added Unit tests

@fayyazarshad fayyazarshad self-assigned this Dec 20, 2019
@coveralls
Copy link

coveralls commented Dec 20, 2019

Pull Request Test Coverage Report for Build 1332

  • 7 of 24 (29.17%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 89.278%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core-api/src/main/java/com/optimizely/ab/Optimizely.java 4 12 33.33%
core-api/src/main/java/com/optimizely/ab/optimizelyconfig/OptimizelyConfig.java 0 9 0.0%
Totals Coverage Status
Change from base Build 1326: -0.2%
Covered Lines: 26229
Relevant Lines: 29379

💛 - Coveralls

@fayyazarshad fayyazarshad force-pushed the fayyaz/integrate-optimizely-config branch from e648907 to 4e9aba8 Compare December 30, 2019 13:45
@fayyazarshad fayyazarshad force-pushed the fayyaz/integrate-optimizely-config branch from 83e89f6 to ce9d305 Compare January 3, 2020 07:39
@zashraf1985 zashraf1985 changed the title Added Caching in Optimizely Config feat: Added caching for OptimizelyConfig object Jan 3, 2020
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mikecdavis mikecdavis left a comment

Choose a reason for hiding this comment

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

LGTM with one request. Let's add the OptimizelyConfigManager to the constructor as a @Nullable parameter and handle the default from the builder. I prefer to keep the constructor devoid of business logic.

@zashraf1985 zashraf1985 merged commit 2e63da9 into master Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants