Add support for custom headers and request-timeout #1070

DJRickyB · 2020-10-05T18:26:07Z

Addresses #567 and #1000
Well it became trivial to add headers and opaque-id support for everything in the course of this refactor so I added that too.

Done:

support request-timeout parameter for most operation types
support opaque-id parameter for most operation types
support headers dict parameter for most operation types
document expanded parameter support

cla-checker-service · 2020-10-05T18:26:10Z

💚 CLA has been signed

DJRickyB · 2020-10-05T18:30:19Z

esrally/driver/runner.py

-                                                       timeout=request_timeout)
-                else:
-                    await es.transport.perform_request("POST", "/_optimize", timeout=request_timeout)
+                await es.transport.perform_request(method="POST", url="/_optimize", params=merge_params)


transport.perform_request() expects the timeout parameter in the params arg. Throws

TypeError: perform_request() got an unexpected keyword argument 'timeout'

it looks like you removed the logic here about max_num_segments. Good catch about the timeout though, lets definitely fix that here once you revert the code change that deleted the if stmt

max_num_segments exists in the merge_params and gets applied to the query string by the perform_request method

see test at https://github.com/elastic/rally/pull/1070/files#diff-026a25af4075b17db2736adf14aa844c0aaeb2bc995b4b4ae94c432b0380b51fR1095

hub-cap · 2020-10-16T18:23:36Z

Hi, @DJRickyB , thank you very much for your contribution! This is good work so far. I have one overall suggestion that might make the code cleaner. It also requires a bit of context.

We hope to one day utilize more than just the timeout in the future, for instance, the opaque-id, so it might make sense to consolidate all of the different Runner implementations to use a single method, such as this method, and use it to populate all of the Runner classes. As is, many of them do some very similar operations with their params to extract the request-params, and maybe we can use a common method to both extract that, as well as ensure we populate the timeout. What do you think of this idea/suggestion?

I have also added just a few minor nits to the pull request that I saw right off the bat, before realizing we could extract the common functionality into the Runners.

hub-cap

Oops! I meant to post that comment into this review, not as just a comment. here are the nits I mentioned in the review itself!

esrally/driver/runner.py

hub-cap · 2020-10-16T18:04:26Z

esrally/driver/runner.py

-                                                       timeout=request_timeout)
-                else:
-                    await es.transport.perform_request("POST", "/_optimize", timeout=request_timeout)
+                await es.transport.perform_request(method="POST", url="/_optimize", params=merge_params)


it looks like you removed the logic here about max_num_segments. Good catch about the timeout though, lets definitely fix that here once you revert the code change that deleted the if stmt

DJRickyB · 2020-10-16T19:52:02Z

Thank you Michael! Your timing is perfect, I was planning on finishing this PR this weekend. Please pardon the force-push, it's not usually my style but I wanted a way to get prior git history to apply to the comment lines I had touched

esrally/driver/runner.py

hub-cap

wow this looks great now! I love how its not touching all the tests. Also, Thank you SO VERY much for making the PR name more descriptive. Just some minor nits now and this will be good to go.

One other thing, go ahead and merge master in git merge upstream/master and run make precommit, and it will show you one line length failure you have. This will fail before the tests will run. Once you are done w this ill make sure our CI infra can run the tests.

esrally/driver/runner.py

hub-cap · 2020-10-19T19:30:21Z

tests/driver/runner_test.py

+            "status": "green",
+            "relocating_shards": 0
+        })
+        r = runner.ClusterHealth()


we try not to have single value names for variables, lets make this a bit more descriptive

tests/driver/runner_test.py

hub-cap · 2020-10-21T18:27:27Z

@elasticmachine ok to test

hub-cap · 2020-10-21T18:28:43Z

grr, my magic incantation is incorrect.

@elasticmachine add to allowlist please

hub-cap · 2020-10-21T18:31:57Z

ok at this point w/ the proper incantation, your PR will now run tests for each commit. If it fails for some reason, go ahead and fix it up (or ask for help if need be), and push, and it should run the tests again.

DJRickyB · 2020-10-21T19:01:36Z

is there somewhere i can push new test tracks to exercise the functionality? good to see it looks like no regressions :)

hub-cap · 2020-10-22T20:42:14Z

If you push some test tracks, it would be rad to just push them to a branch, and then link the commands you use (assuming the repos were checked out locally). Ill test them myself to double check and then we can call this done and merge it. Im off tomorrow, but I should be able to get to them by monday if you push some branches up to your fork of rally tracks.

hub-cap · 2020-10-26T13:41:43Z

Also, I forgot to add, if you are using and existing corpora, you can use the built in rally --test-mode flag which will only ingest a small sample set of data, so your feedback loop is much tighter when testing track changes.

DJRickyB · 2020-10-26T14:41:36Z

Thanks, sorry for the delay. Running with local test tracks and watching with wireshark revealed a couple of misses in params.py based on how parameter parsing for a given operation was set up. The changes I made add 3 None values to the params dictionary for a few types, so now the size check tests have changed in a few places (see failures from your CI).

Edit: going to change the counts tests and update parameters. submitted originally for feedback but the right path became obvious after another cup of coffee

FYI I'm just using this locally: elastic/rally-tracks@d7b1ac4

hub-cap

yay, its merge time!

Currently in polling mode, the force merge operation uses a very short timeout (1s) which, in case of slow response times, might actually cause the operation to not get executed. Since elastic#1070 we can specify per operation request-timeout and we should honor that one instead.

…#1122) Currently in polling mode, the force merge operation uses a very short timeout (1s) which, in case of slow response times, might actually cause the operation to not get executed. Since #1070 we can specify per operation request-timeout and we should honor that one instead. Co-authored-by: Daniel Mitterdorfer <daniel@mitterdorfer.name>

DJRickyB commented Oct 5, 2020

View reviewed changes

dliappis requested a review from hub-cap October 14, 2020 09:55

hub-cap suggested changes Oct 16, 2020

View reviewed changes

initial implementation of broad request-timeout support

6762483

DJRickyB force-pushed the rally-567 branch from dc09504 to 6762483 Compare October 16, 2020 19:45

generalize kw handling from config, minimize test changes

5351234

DJRickyB commented Oct 17, 2020

View reviewed changes

esrally/driver/runner.py Show resolved Hide resolved

add tests, test out generalization by adding broad headers support

e789edd

DJRickyB changed the title ~~Addresses #567~~ Add support for custom headers and request-timeout Oct 19, 2020

add new top-level operation properties to docs

895d263

DJRickyB marked this pull request as ready for review October 19, 2020 03:16

DJRickyB requested a review from hub-cap October 19, 2020 03:16

hub-cap suggested changes Oct 19, 2020

View reviewed changes

Rick Boyd added 2 commits October 19, 2020 16:11

Merge remote-tracking branch 'origin/master' into rally-567

7d24db8

cleanup for PR elastic#1070

e2a6289

DJRickyB requested a review from hub-cap October 20, 2020 22:47

Merge branch 'master' into rally-567

fdd418f

hub-cap self-assigned this Oct 26, 2020

Rick Boyd added 2 commits October 26, 2020 11:03

lint fix

6154245

cover new global es parameters in params.py

111b0b8

DJRickyB force-pushed the rally-567 branch from 56ac0fc to 111b0b8 Compare October 26, 2020 15:04

hub-cap approved these changes Oct 27, 2020

View reviewed changes

hub-cap merged commit 90722ac into elastic:master Oct 27, 2020

hub-cap mentioned this pull request Oct 27, 2020

Ability to specify request timeout per operation #567

Closed

hub-cap added :Usability Makes Rally easier to use enhancement Improves the status quo labels Oct 28, 2020

hub-cap added this to the 2.0.3 milestone Oct 28, 2020

danielmitterdorfer mentioned this pull request Nov 3, 2020

Allow custom headers #1000

Closed

dliappis mentioned this pull request Nov 23, 2020

Remove implicit request timeout from force merge operation in polling #1122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for custom headers and request-timeout #1070

Add support for custom headers and request-timeout #1070

DJRickyB commented Oct 5, 2020 •

edited

Loading

cla-checker-service bot commented Oct 5, 2020 •

edited

Loading

DJRickyB Oct 5, 2020

hub-cap Oct 16, 2020

DJRickyB Oct 16, 2020 •

edited

Loading

hub-cap commented Oct 16, 2020

hub-cap left a comment

hub-cap Oct 16, 2020

DJRickyB commented Oct 16, 2020

hub-cap left a comment

hub-cap Oct 19, 2020

hub-cap commented Oct 21, 2020

hub-cap commented Oct 21, 2020

hub-cap commented Oct 21, 2020

DJRickyB commented Oct 21, 2020

hub-cap commented Oct 22, 2020

hub-cap commented Oct 26, 2020

DJRickyB commented Oct 26, 2020 •

edited

Loading

hub-cap left a comment

Add support for custom headers and request-timeout #1070

Add support for custom headers and request-timeout #1070

Conversation

DJRickyB commented Oct 5, 2020 • edited Loading

cla-checker-service bot commented Oct 5, 2020 • edited Loading

DJRickyB Oct 5, 2020

Choose a reason for hiding this comment

hub-cap Oct 16, 2020

Choose a reason for hiding this comment

DJRickyB Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

hub-cap commented Oct 16, 2020

hub-cap left a comment

Choose a reason for hiding this comment

hub-cap Oct 16, 2020

Choose a reason for hiding this comment

DJRickyB commented Oct 16, 2020

hub-cap left a comment

Choose a reason for hiding this comment

hub-cap Oct 19, 2020

Choose a reason for hiding this comment

hub-cap commented Oct 21, 2020

hub-cap commented Oct 21, 2020

hub-cap commented Oct 21, 2020

DJRickyB commented Oct 21, 2020

hub-cap commented Oct 22, 2020

hub-cap commented Oct 26, 2020

DJRickyB commented Oct 26, 2020 • edited Loading

hub-cap left a comment

Choose a reason for hiding this comment

DJRickyB commented Oct 5, 2020 •

edited

Loading

cla-checker-service bot commented Oct 5, 2020 •

edited

Loading

DJRickyB Oct 16, 2020 •

edited

Loading

DJRickyB commented Oct 26, 2020 •

edited

Loading