Update opts.py to allow for 'None' timeout-value in client_options#1541
Merged
b-deam merged 2 commits intoelastic:masterfrom Jul 11, 2022
Merged
Update opts.py to allow for 'None' timeout-value in client_options#1541b-deam merged 2 commits intoelastic:masterfrom
b-deam merged 2 commits intoelastic:masterfrom
Conversation
…rameter. Added to_none() to opts.py and updated kv_to_map() function.
|
💚 CLA has been signed |
pquentin
requested changes
Jul 8, 2022
Member
pquentin
left a comment
There was a problem hiding this comment.
Thanks! I like the idea. Can you please sign the CLA, document this and add a test in tests/utils/opts_test.py?
pquentin
approved these changes
Jul 8, 2022
Member
pquentin
left a comment
There was a problem hiding this comment.
Thanks for iterating so quickly! This now looks good to me.
We don't merge on Fridays to avoid breaking our nightly benchmarks (https://elasticsearch-benchmarks.elastic.co/), so we'll merge on Monday.
Member
|
Thanks again @MoBoo ! |
Merged
This file contains hidden or 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
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.
Rally sets a default timeout (60s) for every http api call to elasticsearch.
For long running queries this timeout introduces some problems, because rally will fail with a client timeout (which is only traceable via the logs). To work around this timeout issues, one must set the timeout to an arbitrary high value to account for any possible timeouts.
Rally actually supports the following three options for values set in the
--client-optionsparameter:int,floatorboolean. For the timeout value an additionalNoneis required to allow not specifing a timeout at all.This is due to the underlying
urllib3which requires either anint,floatorNonevalue as a timeout as shown by the following error message:.../urllib3/util/timeout.py": ValueError: Timeout value connect was None, but it must be an int, float or None.So far only
intandfloatare usable from the commandline becauseNonecannot be passed via the--client-optionsparameter.This pull request fixes this issue by added the needed functionality to the
opts.pymodule. Ato_none()function is added which parses the String"None"to the pythonicNoneand is called in thekv_to_map()function which parses the--client-optionsparameters.Edit: signed contributor agreement