Retry metric store for more transport errors #538
Merged
Conversation
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
Align Dockerfile with JAVAx_HOME environment variable requirements introduced in: elastic#518
dliappis
changed the title
dliappis
added
enhancement
tests/metrics_test.py
Outdated
|
|
||
| # should return on first success | ||
| operation = mock.Mock(side_effect=[gateway_timeout, gateway_timeout, "success", gateway_timeout]) | ||
| operation = mock.Mock(side_effect=[bad_gateway, bad_gateway, "success", bad_gateway]) |
There was a problem hiding this comment.
Maybe randomize and vary the error type here? I think then we could also reduce the number of individual test cases?
esrally/metrics.py
Outdated
| elif e.status_code == 429 and execution_count < max_execution_count: | ||
| self.logger.debug("Execution rejected in attempt [%d/%d].", execution_count, max_execution_count) | ||
| time.sleep(3) | ||
| retriable_responses_with_sleep = {502: 1, 503: 1, 504: 1, 429: 3} |
There was a problem hiding this comment.
I think it would also be ok if we settle on a single waiting period. Wdyt?
Dockerfile
Outdated
| @@ -25,6 +25,7 @@ ENV HOME /home/${NEW_USER} | |||
| ENV PYENV_ROOT=/home/${NEW_USER}/.pyenv | |||
| ENV PATH=$PYENV_ROOT/shims:$PYENV_ROOT/bin:$PATH | |||
| ENV JAVA_HOME=/opt/jdk-10 | |||
| ENV JAVA10_HOME=/opt/jdk-10 | |||
There was a problem hiding this comment.
This seems unrelated?
There was a problem hiding this comment.
Yes this came from this commit 9ddb393 to fix the it tests in Docker. I'll pull it out and submit separately.
and switch to fixed wait time between attempts (3s).
This will be addressed separately outside of the PR.
|
@danielmitterdorfer Thanks for the review! I changed the logic, as discussed, to use randomized tests and fixed retry periods. Would appreciate another look when you have time. |
danielmitterdorfer
approved these changes
Jul 24, 2018
|
Thank you for the review @danielmitterdorfer ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Improve the resiliency of Rally when using an Elasticsearch remote metrics store by retrying certain transport errors.
Also enhance existing tests to check that retries/debug output and sleep are executed properly.