Skip to content
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

[FSSDK-10763] Implement UPS request batching for decideForKeys #440

Merged
merged 29 commits into from
Nov 27, 2024
Merged

[FSSDK-10763] Implement UPS request batching for decideForKeys #440

merged 29 commits into from
Nov 27, 2024

Conversation

FarhanAnjum-opti
Copy link
Contributor

Summary

  • Currently, decideForKeys loops over the list of flags and calls decide for each, which in turn calls ups. So for decideForKeys, there is two calls per flag to UPS. This PR batches UPS calls when using decideForKeys.

Test plan

  • Existing tests are updated to work with the new change. New tests will be added for batching.

Issues

  • FSSDK-10763

@FarhanAnjum-opti
update: UserProfile class created, changes in decision_service, decid…
0411458 
…e_for_keys
@FarhanAnjum-opti
update: get_variation function changed
43a0c74
@FarhanAnjum-opti
update: new function in decision_service
d462339
@FarhanAnjum-opti
update: everything implemented from java. tests are failing
fc976ff
@FarhanAnjum-opti
update: minor changes
4187366
@FarhanAnjum-opti
update: user_profile_tracker added to tests
8d32180
@FarhanAnjum-opti
update: some tests fixed
ba44a6c
@FarhanAnjum-opti
🛠️ optimizely/decision_service.py -> Added check for `ignore_user_pro…
c78df2f 
…file` in decision logic.

🛠️ optimizely/user_profile.py -> Improved user profile loading with missing key checks.
🛠️ tests/test_decision_service.py -> Updated tests to include user profile tracker.
@FarhanAnjum-opti
🛠️ tests/test_decision_service.py -> Added expected decision object.
d02c3d8 
🛠️ tests/test_decision_service.py -> Updated experiment bucket map call.
🛠️ tests/test_decision_service.py -> Introduced user_profile_tracker usage.
🛠️ tests/test_decision_service.py -> Modified method calls with user_profile_tracker.
@FarhanAnjum-opti
optimizely/decision_service.py -> fixed get_variations_for_feature_list
c67bcf3
@FarhanAnjum-opti
optimizely/decision_service.py -> Fixed how rollout reasons are added
820ba3e 
tests/test_decision_service.py -> Added user profile tracker object
@FarhanAnjum-opti
tests/test_user_context.py -> fixed some tests
309223c
@FarhanAnjum-opti
optimizely/user_profile.py -> Updated type for experiment_bucket_map.
ca1b248 
tests/test_decision_service.py -> Fixed tests
@FarhanAnjum-opti
all unit tests passing
d0bc332
@FarhanAnjum-opti FarhanAnjum-opti requested a review from a team as a code owner November 13, 2024 15:51
Mat001
Mat001 requested changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@Mat001 Mat001 left a comment

Choose a reason for hiding this comment

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

Looks great! Just please cleanup old commented out pieces. Then we'll merge once done :)

optimizely/decision_service.py Outdated Show resolved Hide resolved
optimizely/optimizely.py Outdated Show resolved Hide resolved
optimizely/optimizely.py Outdated Show resolved Hide resolved
optimizely/optimizely.py Outdated Show resolved Hide resolved
optimizely/user_profile.py Outdated Show resolved Hide resolved
optimizely/user_profile.py Outdated Show resolved Hide resolved
tests/test_user_context.py Outdated Show resolved Hide resolved
raju-opti
raju-opti requested changes Nov 21, 2024
View reviewed changes
optimizely/decision_service.py Outdated Show resolved Hide resolved
optimizely/optimizely.py Show resolved Hide resolved
optimizely/optimizely.py Show resolved Hide resolved
@FarhanAnjum-opti
optimizely/decision_service.py -> Removed user profile save logic
9228727 
optimizely/optimizely.py -> Added loading and saving profile logic
@FarhanAnjum-opti
optimizely/user_profile.py -> Updated experiment_bucket_map type
b33a92b 
optimizely/user_profile.py -> Testing user profile update logic
@FarhanAnjum-opti
optimizely/decision_service.py -> Commented out profile loading
1dd1eef 
optimizely/user_profile.py -> Removed unused import statement
@FarhanAnjum-opti
optimizely/decision_service.py -> Removed unused profile loading
6387ca2 
optimizely/user_profile.py -> Fixed handling of reasons list
optimizely/user_profile.py -> Improved profile retrieval error logging
tests/test_decision_service.py -> Updated mock checks to simplify tests
tests/test_user_profile.py -> Added tests for user profile handling
tests/test_optimizely.py -> New test for variation lookup and save
Mat001
Mat001 approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@Mat001 Mat001 left a comment

Choose a reason for hiding this comment

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

I think it looks ok to me!

raju-opti
raju-opti approved these changes Nov 27, 2024
View reviewed changes
Copy link

@raju-opti raju-opti left a comment

Choose a reason for hiding this comment

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

lgtm. two non critical comments

optimizely/user_profile.py Show resolved Hide resolved
optimizely/user_profile.py Show resolved Hide resolved
@FarhanAnjum-opti FarhanAnjum-opti merged commit 22c74ee into master Nov 27, 2024
23 checks passed
@FarhanAnjum-opti FarhanAnjum-opti deleted the farhan/FSSDK-10763-batch-UPS branch November 27, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raju-opti raju-opti raju-opti approved these changes

@Mat001 Mat001 Mat001 approved these changes

Assignees

@FarhanAnjum-opti FarhanAnjum-opti

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FarhanAnjum-opti @Mat001 @raju-opti