-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Diff ABT Experiments for real-time RC #11398
Closed
Closed
Changes from 29 commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
f853b61
Add activated experiment metadata and payload keys to experiments table
qdpham13 9f1cea0
Call updateActivatedExperiments in activation method
qdpham13 ceae355
Update key name for experiments table
qdpham13 e7b92b1
Update test to include activation call
qdpham13 db6aff1
Update DB manager for Active experiments payload and metadata
qdpham13 5867edb
Fix metadata naming
qdpham13 c2b6097
add comment
qdpham13 d1b2349
Merge branch 'master'
qdpham13 937630d
Don't save experiments metadata b/c it isn't required to diff experim…
qdpham13 d49bffb
Create diff method
qdpham13 0067548
Adding diffing logic between fetched and activated experiments
qdpham13 d02b9e5
formatting
qdpham13 c9acee2
Refactor more of main diffing method
qdpham13 8337c60
use ABT diffing in ConfigUpdate logic
qdpham13 12be9c7
Add extra assertion for test
qdpham13 69d4061
Merge branch 'abt-exp-meta'
qdpham13 92efb59
Add tests
qdpham13 bac29da
Add in commenting
qdpham13 829d953
Address PR comments
qdpham13 9a0f4e6
update naming
qdpham13 bc78a5e
Merge branch 'abt-exp-meta'
qdpham13 707f5f1
Merge branch 'master'
qdpham13 0ce47b9
formatting
qdpham13 5166676
Update diffing logic
qdpham13 bf27372
Merge branch 'master'
qdpham13 756df3b
format and add comments
qdpham13 a1a01e6
Fix grammar
qdpham13 00d8284
Update comment
qdpham13 693a2de
Remove unused constant
qdpham13 9275933
Move diffing logic to ConfigContainer
qdpham13 b68a3df
remove space
qdpham13 6dbc9c6
Delete TestABTPayload2.txt
qdpham13 4e4f4d9
Merge branch 'master'
qdpham13 e365cce
Merge remote-tracking branch 'refs/remotes/origin/abt-exp-diff-2'
qdpham13 6e12a8c
Make it easier to test
qdpham13 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,5 +15,6 @@ | |
{ | ||
"experimentId": "exp_1" | ||
} | ||
] | ||
], | ||
"affectedParameterKeys": ["test_key_1"] | ||
} |
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.
instead of doing the diffing elsewhere and passing in the changed keys, could we not read the experiment metadata directly from the dbManager here and do the diffing in this method?
feels unnecessary to have to explicitly pass in the experiment-related keys to
getConfigUpdateForNamespace
, when the diffing logic can be self-contained withinRCNConfigContent
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.
My thinking is that all experiment logic lives in RCNConfigExperiments including in-memory copies of active and fetched experiments so why not do the diffing where all of the experiments already are so that we don't need to load all of the data elsewhere to do the same thing. Plus where it's called in Fetch allows us to easily pass in the keys after getting the diffs from experiments
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.
But I could go either way
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.
I get that it works as-is, but you are having the
Fetch
class partially compute the results, then pass it to theConfig
class, just to have it passed back.Fetch
doesn't/shouldn't need to know the details of how theConfigUpdate
is computed, otherwise in the future if we decide different metadata is needed (which will likely happen soon), fetch (or anywhere else we call this method) may need to be updated