Allow index pattern for source-index in shrink-index operation #1118

dliappis · 2020-11-19T20:40:43Z

This commit adds support for specifying several source indices using the
standard Elasticsearch Multi-target syntax for the shrink-index
runner.

This helps automate shrink operations for multiple indices.

This commit adds support for specifying several source indices using the standard Elasticsearch Multi-target syntax for the shrink-index runner. This helps automate shrink operations for multiple indices.

dliappis · 2020-11-19T20:43:40Z

esrally/driver/runner.py

+            await es.indices.shrink(index=source_index, target=final_target_index, body=target_body)
+
+            self.logger.info("Waiting for shrink to finish for index [%s] ...", source_index)
+            await self._wait_for(es, final_target_index, "shrink for index [{}]".format(final_target_index))


We could run things in parallel by moving this to a separate loop later, however, I am not sure whether a high level of concurrent shrink operations will actually be beneficial or cause contention and provide minimal advantage (esp. if only one shrink node has been provided).

Agreed, I'd not complicate things unless there is a strong benefit.

danielmitterdorfer

Nice convenience improvement. Looks fine for the most part but I left a few comments.

danielmitterdorfer · 2020-11-20T08:15:42Z

esrally/driver/runner.py

        # we need to inject additional settings so we better copy the body
        target_body = deepcopy(mandatory(params, "target-body", self))
        shrink_node = params.get("shrink-node")
        # Choose a random data node if none is specified
+        node_names = [shrink_node] if shrink_node else []


I think this makes the code a bit hard to read as you operate on two variables (node_names and shrink_node) in several branches and check shrink_node multiple times.

How about this instead?

# Choose a random data node if none is specified if shrink_node: node_names = [shrink_node] else: node_names = [] # choose a random data node node_info = await es.nodes.info() for node in node_info["nodes"].values(): if "data" in node["roles"]: node_names.append(node["name"]) if not node_names: raise exceptions.RallyAssertionError("Could not choose a suitable shrink-node automatically. Please specify it explicitly.")

This makes it easier to reason because there is only one top-level conditional and then we populate node_names in each branch separately.

Addressed in a3cab0e (also removed please following https://developers.google.com/style/tone#politeness-and-use-of-please)

esrally/driver/runner.py

danielmitterdorfer · 2020-11-20T08:19:51Z

esrally/driver/runner.py

+            f"Add it to your parameter source and try again.")
+
+
+def remove_prefix(string, prefix):


Too bad we don't have Python 3.9 as minimum version requirement as this has been recently added to the standard library. I wonder whether we should put a TODO into the code?

Yes I noticed that too while coding writing this. Added a TODO in a3cab0e.

danielmitterdorfer · 2020-11-20T08:24:38Z

esrally/driver/runner.py

+                                          preserve_existing=True)
+
+            self.logger.info("Waiting for relocation to finish for index [%s] ...", source_index)
+            await self._wait_for(es, source_index, "shard relocation for index [{}]".format(source_index))


We could use an f-string here?

Done in 52f28a6

danielmitterdorfer · 2020-11-20T08:25:50Z

esrally/driver/runner.py

+            await es.indices.shrink(index=source_index, target=final_target_index, body=target_body)
+
+            self.logger.info("Waiting for shrink to finish for index [%s] ...", source_index)
+            await self._wait_for(es, final_target_index, "shrink for index [{}]".format(final_target_index))


We could use an f-string here?

Done in a3cab0e

danielmitterdorfer · 2020-11-20T08:26:18Z

esrally/driver/runner.py

+            await es.indices.shrink(index=source_index, target=final_target_index, body=target_body)
+
+            self.logger.info("Waiting for shrink to finish for index [%s] ...", source_index)
+            await self._wait_for(es, final_target_index, "shrink for index [{}]".format(final_target_index))


Agreed, I'd not complicate things unless there is a strong benefit.

danielmitterdorfer · 2020-11-20T08:26:50Z

esrally/driver/runner.py

+
+            self.logger.info("Waiting for shrink to finish for index [%s] ...", source_index)
+            await self._wait_for(es, final_target_index, "shrink for index [{}]".format(final_target_index))
+            self.logger.info("Shrinking [%s] to [%s] has finished.", source_index, final_target_index)
        # ops_count is not really important for this operation...
        return 1, "ops"


I think we could use the number of shrunk indices here for the operation count?

Done in a3cab0e

danielmitterdorfer · 2020-11-20T08:29:20Z

docs/track.rst

-* ``source-index`` (mandatory): The name of the index that should be shrinked.
-* ``target-index`` (mandatory): The name of the index that contains the shrinked shards.
+* ``source-index`` (mandatory): The name of the index that should be shrinked. Multiple indices can be defined using the `Multi-target syntax <https://www.elastic.co/guide/en/elasticsearch/reference/current/multi-index.html>`_.
+* ``target-index`` (mandatory): The name of the index that contains the shrinked shards. If multiple indices match ``source-index``, the common prefix will appended to ``target-index``.


Isn't it the other way around? The common prefix will be removed and instead the unique parts of each source index name will be appended to the string specified via target-index?

Of course 🤦 ! Addressed in a3cab0e

dliappis · 2020-11-20T09:10:00Z

@danielmitterdorfer Thanks for the review! I addressed your comments and requested a second review.

danielmitterdorfer

Thanks! LGTM

danielmitterdorfer · 2020-11-20T09:42:35Z

docs/track.rst

@@ -1303,7 +1303,7 @@ shrink-index
 With the operation ``shrink-index`` you can execute the `shrink index API <https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-shrink-index.html>`_. Note that this does not correspond directly to the shrink index API call in Elasticsearch but it is a high-level operation that executes all the necessary low-level operations under the hood to shrink an index. It supports the following parameters:

 * ``source-index`` (mandatory): The name of the index that should be shrinked. Multiple indices can be defined using the `Multi-target syntax <https://www.elastic.co/guide/en/elasticsearch/reference/current/multi-index.html>`_.
-* ``target-index`` (mandatory): The name of the index that contains the shrinked shards. If multiple indices match ``source-index``, the common prefix will appended to ``target-index``.
+* ``target-index`` (mandatory): The name of the index that contains the shrinked shards. If multiple indices match ``source-index``, one shrink operation will execute for every matching index. Each shrink operation will use a modified ``target-index``: the unique suffix of the source index (derived by removing the common prefix of all matching source indices) will be appended to ``target-index``. See also the example below.


Much clearer, thanks!

Allow index pattern for source-index in shrink-index operation

dbc63cb

This commit adds support for specifying several source indices using the standard Elasticsearch Multi-target syntax for the shrink-index runner. This helps automate shrink operations for multiple indices.

dliappis added enhancement Improves the status quo :Track Management New operations, changes in the track format, track download changes and the like labels Nov 19, 2020

dliappis added this to the 2.0.3 milestone Nov 19, 2020

dliappis requested a review from danielmitterdorfer November 19, 2020 20:40

dliappis self-assigned this Nov 19, 2020

dliappis commented Nov 19, 2020

View reviewed changes

danielmitterdorfer reviewed Nov 20, 2020

View reviewed changes

dliappis added 2 commits November 20, 2020 11:03

Address PR comments

a3cab0e

Use f-string

52f28a6

dliappis requested a review from danielmitterdorfer November 20, 2020 09:09

danielmitterdorfer approved these changes Nov 20, 2020

View reviewed changes

dliappis merged commit b8e8e6b into elastic:master Nov 20, 2020

dliappis deleted the shrink-index-pattern branch November 20, 2020 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow index pattern for source-index in shrink-index operation #1118

Allow index pattern for source-index in shrink-index operation #1118

dliappis commented Nov 19, 2020

dliappis Nov 19, 2020

danielmitterdorfer Nov 20, 2020

danielmitterdorfer left a comment

danielmitterdorfer Nov 20, 2020

dliappis Nov 20, 2020

danielmitterdorfer Nov 20, 2020

dliappis Nov 20, 2020

danielmitterdorfer Nov 20, 2020

dliappis Nov 20, 2020

danielmitterdorfer Nov 20, 2020

dliappis Nov 20, 2020

danielmitterdorfer Nov 20, 2020

danielmitterdorfer Nov 20, 2020

dliappis Nov 20, 2020

danielmitterdorfer Nov 20, 2020

dliappis Nov 20, 2020

dliappis commented Nov 20, 2020

danielmitterdorfer left a comment

danielmitterdorfer Nov 20, 2020

		f"Add it to your parameter source and try again.")


		def remove_prefix(string, prefix):

Allow index pattern for source-index in shrink-index operation #1118

Allow index pattern for source-index in shrink-index operation #1118

Conversation

dliappis commented Nov 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmitterdorfer 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

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

dliappis commented Nov 20, 2020

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment