Force Merge Runner Improvements - Polling #1051

gingerwizard · 2020-08-21T16:04:49Z

This adds a polling mode to the force-merge runner and in the process closes #1050

Some considerations:

The change adds a mode parameter. This defaults to blocking which causes the current behaviour.
If set to polling we ignore the request-timeout parameter and timeout the force_merge API call after 1s. We then poll the tasks API at an interval of poll-period (default 10s), until no force-merge tasks are listed.
Currently there is no timeout on the tasks API calls. It is possible this is susceptible to cluster pressure and may itself timeout under heavy load. However, i believe this represents a valid error condition.
The 1s timeout is a workaround to the force_merge API not supporting an async call and returning a task id. If multiple force_merge calls are issued in different runners in parallel, all force merges must complete to unblock. This seems low risk as consecutive calls are effectively a noop if max-num-segments has been reached.

Will add docs once parameter naming and usage is agreed.

elastic#1049) * Hints for handling errors and identifying queries and responses * Fix formatting errors * Fix links and formatting * Grammatical fixes

gingerwizard · 2020-08-24T09:36:40Z

@dliappis This is ready for an initial review now all CI tests pass.

dliappis

Thank you for this super useful addition!

I left a number of style comments (unfortunately we skip many errors that pylint would normally report, see #838)

I haven't tested this yet, will do this asap.

dliappis · 2020-08-25T15:34:29Z

esrally/driver/runner.py

@@ -618,11 +617,28 @@ async def __call__(self, es, params):
        # the raw transport API (where the keyword argument is called `timeout`) in some cases we will always need
        # a special handling for the force-merge API.
        request_timeout = params.get("request-timeout")
+        mode = params.get("mode")
+        merge_params = { "request_timeout": request_timeout}


nit: https://www.flake8rules.com/rules/E201.html: merge_params = {"request_timeout": request_timeout} (IDE should want you about this, unfortunately #838 isn't done yet to help us in CI).

Also line 608 needs an additional whitespace, see https://www.flake8rules.com/rules/E302.html

dliappis · 2020-08-25T15:39:51Z

esrally/resources/track-schema.json

+          "mode": {
+            "type": "string",
+            "enum": ["blocking", "polling"],
+            "description": "[Only for type 'force-merge']: Determines whether forced merged is blocking, causing a potential client timeout, or if it polls until no further force merge tasks."


s/force merged/force merge

dliappis · 2020-08-25T15:40:36Z

esrally/track/params.py

@@ -609,7 +609,6 @@ def _init_internal_params(self):
    def percent_completed(self):
        return self.current_bulk / self.total_bulks

-
 class ForceMergeParamSource(ParamSource):


https://www.flake8rules.com/rules/E302.html

tests/driver/runner_test.py

dliappis · 2020-08-25T15:44:29Z

tests/driver/runner_test.py

+                }
+            }),
+            as_future({
+                "nodes":{}


https://www.flake8rules.com/rules/E231.html

dliappis · 2020-08-25T15:45:20Z

tests/driver/runner_test.py

+            })
+        ]
+        force_merge = runner.ForceMerge()
+        await force_merge(es, params={"index" : "_all", "mode": "polling", 'poll-period': 0})


https://www.flake8rules.com/rules/E203.html ; also let's stick to double quotes (it's a convention in the Rally codebase).

dliappis · 2020-08-25T15:45:50Z

tests/driver/runner_test.py

+                }
+            }),
+            as_future({
+                "nodes":{}


https://www.flake8rules.com/rules/E231.html

dliappis · 2020-08-25T15:46:23Z

tests/driver/runner_test.py

+        force_merge = runner.ForceMerge()
+        # request-timeout should be ignored as mode:polling
+        await force_merge(es, params={"index" : "_all", "mode": "polling", "max-num-segments": 1,
+                                      "request-timeout": 50000, 'poll-period': 0})


Similar comment here: https://www.flake8rules.com/rules/E203.html and double quotes instead of single quotes.

dliappis · 2020-08-25T15:47:35Z

tests/track/params_test.py

@@ -1811,7 +1811,6 @@ def test_replaces_body_params(self):

        self.assertNotEqual(first, second)

-
 class ForceMergeParamSourceTests(TestCase):


https://www.flake8rules.com/rules/E302.html

tests/track/params_test.py

…ced_merge

gingerwizard · 2020-08-31T16:02:55Z

@dliappis i believe i have addressed the formatting issues. If you're happy with the approach i'll add docs.

…lastic#1055) When race didn't have total_transform_processing_times to report, `esrally compare` fails Relates to elastic#1022

dliappis · 2020-09-09T08:49:35Z

docs/recipes.rst

+	  "operation" : "scroll",
+	  "operation-type" : "Search"
+	}
+


FYI These doc fragmants seem to have come from 89ed934 (they've already been merged in #1049).

@gingerwizard I still see this unrelated recipe in docs (coming from the older PR).

dliappis

@gingerwizard I tested this yesterday in a real long benchmark where force merging down to 1 segment takes a very long time the polling mode worked marvelously. Let's proceed with docs.

…ced_merge

dliappis

Have you added docs already for the polling mode of the force merge runner? I didn't see anything in this review cycle.

dliappis · 2020-09-14T07:01:38Z

docs/recipes.rst

+	  "operation" : "scroll",
+	  "operation-type" : "Search"
+	}
+


@gingerwizard I still see this unrelated recipe in docs (coming from the older PR).

dliappis · 2020-09-14T07:03:33Z

@elasticmachine test this

dliappis · 2020-09-14T07:04:04Z

@elasticmachine run tests

Dale McDiarmid added 7 commits August 20, 2020 12:28

Hints for handling errors and identifying queries and responses

8aa9283

Fix formatting errors

5db75b5

Initial Force merge Improvements

b08b8e0

Fix links and formatting

7419fcf

Grammatical fixes

e2ec43d

First iteration of force merge for - Closes issue elastic#1050

eb69faf

Add parameter tests

0cb7bb6

gingerwizard added the enhancement Improves the status quo label Aug 21, 2020

gingerwizard added this to the 2.0.2 milestone Aug 21, 2020

gingerwizard requested a review from dliappis August 21, 2020 16:04

gingerwizard self-assigned this Aug 21, 2020

Dale McDiarmid added 3 commits August 21, 2020 17:48

Docs - hints for handling errors and identifying queries and responses (

030f71b

elastic#1049) * Hints for handling errors and identifying queries and responses * Fix formatting errors * Fix links and formatting * Grammatical fixes

Remove redundant line

c6a14ca

Wrap line

fb92c71

dliappis requested a review from danielmitterdorfer August 24, 2020 16:02

dliappis requested changes Aug 25, 2020

View reviewed changes

Dale McDiarmid added 3 commits August 31, 2020 16:37

Merge branch 'master' of github.com:gingerwizard/rally into forced_merge

89ed934

Merge branch 'forced_merge' of github.com:gingerwizard/rally into for…

b888bff

…ced_merge

Fix formatting issues

055b4a9

gingerwizard requested a review from dliappis August 31, 2020 16:02

Check if total_transform_processing_times exist for compare reports (e…

77d193c

…lastic#1055) When race didn't have total_transform_processing_times to report, `esrally compare` fails Relates to elastic#1022

dliappis reviewed Sep 9, 2020

View reviewed changes

dliappis self-requested a review September 10, 2020 06:31

dliappis reviewed Sep 10, 2020

View reviewed changes

gingerwizard requested a review from dliappis September 11, 2020 13:59

Dale McDiarmid added 3 commits September 11, 2020 15:12

Initial Force merge Improvements

7a5882e

First iteration of force merge for - Closes issue elastic#1050

c7ea6de

Add parameter tests

3c44d82

Dale McDiarmid added 4 commits September 11, 2020 15:15

Remove redundant line

7e30955

Wrap line

5806cb6

Fix formatting issues

9b967a5

Merge branch 'forced_merge' of github.com:gingerwizard/rally into for…

4ccda9a

…ced_merge

dliappis reviewed Sep 14, 2020

View reviewed changes

gingerwizard closed this Sep 14, 2020

gingerwizard mentioned this pull request Sep 14, 2020

Polling force merge #1057

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force Merge Runner Improvements - Polling #1051

Force Merge Runner Improvements - Polling #1051

gingerwizard commented Aug 21, 2020

gingerwizard commented Aug 24, 2020

dliappis left a comment

dliappis Aug 25, 2020

dliappis Aug 25, 2020

dliappis Aug 25, 2020

dliappis Aug 25, 2020

dliappis Aug 25, 2020

dliappis Aug 25, 2020

dliappis Aug 25, 2020

dliappis Aug 25, 2020

dliappis Aug 25, 2020

gingerwizard commented Aug 31, 2020

dliappis Sep 9, 2020

dliappis Sep 14, 2020

dliappis left a comment

dliappis left a comment

dliappis Sep 14, 2020

dliappis commented Sep 14, 2020

dliappis commented Sep 14, 2020

		@@ -1811,7 +1811,6 @@ def test_replaces_body_params(self):

		self.assertNotEqual(first, second)


		class ForceMergeParamSourceTests(TestCase):

Force Merge Runner Improvements - Polling #1051

Force Merge Runner Improvements - Polling #1051

Conversation

gingerwizard commented Aug 21, 2020

gingerwizard commented Aug 24, 2020

dliappis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gingerwizard commented Aug 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dliappis left a comment

Choose a reason for hiding this comment

dliappis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dliappis commented Sep 14, 2020

dliappis commented Sep 14, 2020