-
Notifications
You must be signed in to change notification settings - Fork 429
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
fix: Speed up identity overrides #4840
Merged
Merged
Changes from 29 commits
Commits
Show all changes
63 commits
Select commit
Hold shift + click to select a range
49142b0
Query only one page of identity overrides for features
zachaysan 9e8d767
Pass feature ids to get_edge_identity_overrides
zachaysan 8d864f2
Paginate the view to get the feature ids for the context
zachaysan 5a5e7e4
Add a test that get_overrides_data gets passed the feature ids
zachaysan 80f6588
simplify caching logic
matthewelwell 678ddd4
Update test to pass
zachaysan 79d46d5
Merge branch 'fix/speed_up_identity_overrides' of github.com:Flagsmit…
zachaysan 33814e5
Add warning docstring
zachaysan 6d0c046
Update mock assertion
zachaysan cdf378d
Limit based on whether feature ids have been passed in
zachaysan 1a8393b
Remove obsolete argument
zachaysan 415cb51
Add more identity overrides boolean
zachaysan 9044edb
Remove obsolete arg and set more identity overrides
zachaysan a470918
Set more identity overrides boolean
zachaysan 4fc6b50
Add get_identity_overrides_key_condition_expression and remove argument
zachaysan d9b52b5
Add more identity overrides
zachaysan 9854471
Remove obsolete argument
zachaysan 3e2c959
Add test for searching based on feature ids
zachaysan 81b402e
Merge branch 'main' into fix/speed_up_identity_overrides
zachaysan 26cd0c9
Remove as_completed
zachaysan a8c3719
Create get_edge_identity_overrides_for_feature_ids
zachaysan 5a9c5e8
Remove more_identities_overrides and create IdentityOverridesV2List
zachaysan 1c15e36
Use IdentityOverridesQueryResponse
zachaysan 0d84428
Switch to is_num_identity_overrides_complete
zachaysan 15a7138
Update caller to new identities overrides response from client
zachaysan 47b2f1c
Update help text and switch to is_num_identity_overrides_complete
zachaysan 001ca64
Update call assertion
zachaysan 82614bb
Switch to is_num_identity_overrides_complete and test page of identit…
zachaysan 6b74a68
Run test with feature_ids
zachaysan f1a91b1
Switch from NOTE to TODO with issue linked
zachaysan 0c620a8
Page non-deterministic results
zachaysan b151941
Merge branch 'main' into fix/speed_up_identity_overrides
zachaysan aea8974
Query only one page of identity overrides for features
zachaysan 9c37f6f
Pass feature ids to get_edge_identity_overrides
zachaysan ed79cab
Paginate the view to get the feature ids for the context
zachaysan 4ad8e16
Add a test that get_overrides_data gets passed the feature ids
zachaysan 95a2e61
Update test to pass
zachaysan 036dcdb
simplify caching logic
matthewelwell 2159ace
Add warning docstring
zachaysan 97dfdf9
Update mock assertion
zachaysan ab56777
Limit based on whether feature ids have been passed in
zachaysan 9c242aa
Remove obsolete argument
zachaysan a975129
Add more identity overrides boolean
zachaysan b684fa4
Remove obsolete arg and set more identity overrides
zachaysan beeefae
Set more identity overrides boolean
zachaysan 6bde39a
Add get_identity_overrides_key_condition_expression and remove argument
zachaysan 0cbb298
Add more identity overrides
zachaysan 71cee1e
Remove obsolete argument
zachaysan eaa20f6
Add test for searching based on feature ids
zachaysan 55ca9ca
Remove as_completed
zachaysan 9a46b7a
Create get_edge_identity_overrides_for_feature_ids
zachaysan 98c1401
Remove more_identities_overrides and create IdentityOverridesV2List
zachaysan 6dff803
Use IdentityOverridesQueryResponse
zachaysan 9b1ec2d
Switch to is_num_identity_overrides_complete
zachaysan 9de188a
Update caller to new identities overrides response from client
zachaysan 1083745
Update help text and switch to is_num_identity_overrides_complete
zachaysan d7d6133
Update call assertion
zachaysan 2ce37df
Switch to is_num_identity_overrides_complete and test page of identit…
zachaysan 2513d83
Run test with feature_ids
zachaysan 59d2648
Switch from NOTE to TODO with issue linked
zachaysan 6448a70
Page non-deterministic results
zachaysan ec70099
Fix conflicts and merge branch 'main' into fix/speed_up_identity_over…
zachaysan 32224e9
Fix conflicts and merge branch 'fix/speed_up_identity_overrides' of g…
zachaysan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Since the PR has been approved this is a non-blocking comment but the polymorphic return type does look like a code smell.
At this point, factoring the
feature_ids is not None
branch to a separate method looks like a good solution.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.
Talked with @matthewelwell about it and we're going to leave that change to a subsequent PR.