Skip to content
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

Update opts.py to allow for 'None' timeout-value in client_options #1541

Merged
merged 2 commits into from
Jul 11, 2022

Conversation

MoBoo
Copy link
Contributor

@MoBoo MoBoo commented Jul 7, 2022

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-options parameter: int, float or boolean. For the timeout value an additional None is required to allow not specifing a timeout at all.
This is due to the underlying urllib3 which requires either an int, float or None value 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 int and float are usable from the commandline because Nonecannot be passed via the --client-options parameter.

This pull request fixes this issue by added the needed functionality to the opts.py module. A to_none() function is added which parses the String "None" to the pythonic None and is called in the kv_to_map() function which parses the --client-options parameters.

Edit: signed contributor agreement

…rameter.

Added to_none() to opts.py and updated kv_to_map() function.
@cla-checker-service
Copy link

cla-checker-service bot commented Jul 7, 2022

💚 CLA has been signed

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I like the idea. Can you please sign the CLA, document this and add a test in tests/utils/opts_test.py?

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@b-deam b-deam merged commit b00f4f0 into elastic:master Jul 11, 2022
@b-deam
Copy link
Member

b-deam commented Jul 11, 2022

Thanks again @MoBoo !

@b-deam b-deam mentioned this pull request Jul 11, 2022
@pquentin pquentin added this to the 2.6.1 milestone Jul 26, 2022
@pquentin pquentin added the enhancement Improves the status quo label Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the status quo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants