-
Notifications
You must be signed in to change notification settings - Fork 313
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
Change filestore to be indexed by unique ID #720
Conversation
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 looks like a well contained and solid change to me, thanks!
A few additional things:
-
adjust the example in https://github.com/elastic/rally/blob/master/docs/docker.rst where we show
tree myrally/benchmarks/races/
andhead -4 benchmarks/races/2019-06-05-13-51-20/race.json
to use the new format with the race id. -
add a note in https://github.com/elastic/rally/blob/master/docs/migrate.rst about the this change in behavior starting with the next version.
-
Please also remember to add the milestone (e.g.
1.3.0
) as well as related labels to the PR and when it's time to merge, ensure the merge commit hasrelates ...
references to this PR as well as the issue it's addressing (Allow to manage Elasticsearch nodes separately from benchmarking #697), e.g.:Relates #697 Relates #720
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 left a comment as I don't fully understand why we need the latest commit 527e93a
We'll still need the changes in the docs mentioned here: #720 (review)
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 for iterating! LGTM
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 think there is one bit missing: A workflow that we support is that the user retrieves the trial id (usually via esrally list races
) and then compares two trial ids (esrally compare --baseline=... --contender=...
). But at the moment this does not work. Listing races still shows timestamps but they cannot be used when comparing. Also, the compare
subcommand needs to search via trial id now. I think it would be nice though if we would still show the trial timestamp in addition in the list
subcommand (in the second column). Note that this also requires changing the respective docs in https://esrally.readthedocs.io/en/stable/tournament.html.
@drawlerr c8f0d82 and 4131665 look good to me, thanks! The tournament docs still need updating. |
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 for iterating. It looks mostly fine now; I just have a couple of minor comments.
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 for iterating. The changes look good to me now. LGTM :)
With this commit we store the race file always as `race.json`. Previously Rally had a logic built-in to create a unique file name if there was a potential for duplicate race files (with the same race timestamp) but as we're using the race id now for the base directory this is not possible anymore. Relates elastic#720
With this commit we store the race file always as `race.json`. Previously Rally had a logic built-in to create a unique file name if there was a potential for duplicate race files (with the same race timestamp) but as we're using the race id now for the base directory this is not possible anymore. Relates #720 Relates #773
With this commit we store the race file always as `race.json`. Previously Rally had a logic built-in to create a unique file name if there was a potential for duplicate race files (with the same race timestamp) but as we're using the race id now for the base directory this is not possible anymore. Relates elastic#720 Relates elastic#773
Races in the filestore will now be stored by their Trial ID, not their timestamp.
Relates #697