-
Notifications
You must be signed in to change notification settings - Fork 35
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
Use existing timestamp as experiment id #62
Conversation
This commit introduces two changes. First, it changes type of experiment id from UUID to timestamp, mainly to reuse existing id and improve readability of events. This simplifies experiment analysis when the db contains results from many tests. Second, it adds initital test framework to validate event stream generate by benchmark executor. The test does not really execute any sql statements. A mock simulates the execution so that just the executor's behavior can be validated. Addresses microsoft#53
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.
Thanks @ashvina . It looks like there are a few places in this patch where some methods/variables aren't being used yet, I'm not sure if that was intentional or an oversight when cherry-picking the contents for this PR. Could we remove those from this patch? Alternatively, you can push a more complete version of this patch; in fact, the patch is not too large to review.
An additional comment based on new methods in EventInfo
(even though they may not be part of this patch based on the comment above). Since it seems the overlap between EventInfo
and new EventInfo
is not significant, maybe a better option altogether would be to create a new EventInfo
and deprecating the old one including related events; we can keep the old version around and log both EventInfo
until metrics module is changed. WDYT?
src/test/java/com/microsoft/lst_bench/common/LSTBenchmarkExecutorTest.java
Show resolved
Hide resolved
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.
Left a comment about Nullable field. Other than that, LGTM. +1
This commit introduces two changes.
First, it changes type of experiment id from UUID to timestamp, mainly to reuse existing id and improve readability of events. This simplifies experiment analysis when the db contains results from many tests.
Second, it adds initial test framework to validate event stream generate by benchmark executor. The test does not really execute any sql statements. A mock simulates the execution so that just the executor's behavior can be validated.
Addresses #53