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.
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
add experiment logic #1081
add experiment logic #1081
Changes from all commits
216a00e
d8b385c
914a5ed
93247a3
5d530f2
2dcef1e
5aec88d
b188d67
1054923
c05aa96
8387472
e862ad8
6090a15
14f92e9
8bda08c
9a19db2
56c54a7
86b7c97
2595fcf
86d6f7b
95c4470
c30f0b4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
looking at this API, I see an important issue with the naming. I understand the urge to be backward compatible, but I dislike that a method named like a simple getter actually mutates the internal state underneath (this is new behavior). can you think of proposing something here? e.g. resolveState(for subfeature?
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.
You should limit the sync queue block to only what truly requires synchronization. Avoid including any dependencies inside it (!). For example, the InternalUserDecider should handle its responsibilities elsewhere because if someone adds main queue synchronization code to it ever, it will lead to potential deadlock
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.
lint issue
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.
What I dislike the most is that AppPrivacyConfiguration is turning into a MassiveAppPrivacyConfiguration. Can we plan a follow-up project to streamline and clean this up, possibly?
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.
this queue is not needed, it gives a false sense of security but it doesn't protect from anything;
instead you should clarify in the documentation for this manager that it is inherently not thread-safe, particularly for the method that assigns a cohort (as it mutates the state), and so should never be used elsewhere except for as an instance inside AppPrivacyConfig