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.

Sign up for GitHub

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

Jump to bottom

Bulk action refresh param #1743

Merged
merged 7 commits into from
Jul 10, 2023
Merged

Bulk action refresh param #1743

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
41b4c30
Support the refresh track parameter for bulk requests
inqueue Jun 14, 2023
c23c975
Add refresh parameter tests
inqueue Jun 29, 2023
4f5a2a0
Support three refresh parameter values
inqueue Jul 6, 2023
2429aa4
Linting fixes
inqueue Jul 6, 2023
b1a822c
Use actual refresh parameter values
inqueue Jul 7, 2023
49f7a64
Update tests for expected values
inqueue Jul 7, 2023
bdfd1d8
Raise RallyAssertionError with an invalid refresh value
inqueue Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/track.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,7 +757,7 @@ Properties
* ``recency`` (optional, defaults to 0): A number between [0,1] indicating whether to bias conflicting ids towards more recent ids (``recency`` towards 1) or whether to consider all ids for id conflicts (``recency`` towards 0). See the diagram below for details.
* ``detailed-results`` (optional, defaults to ``false``): Records more detailed meta-data for bulk requests. As it analyzes the corresponding bulk response in more detail, this might incur additional overhead which can skew measurement results. See the section below for the meta-data that are returned. This property must be set to ``true`` for individual bulk request failures to be logged by Rally.
* ``timeout`` (optional, defaults to ``1m``): Defines the `time period that Elasticsearch will wait per action <https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html#docs-bulk-api-query-params>`_ until it has finished processing the following operations: automatic index creation, dynamic mapping updates, waiting for active shards.
* ``refresh`` (optional): Control Elasticsearch refresh behavior for bulk requests. Valid values are ``async``, ``sync``, and ``default``. Parameter values are mapped to their bulk API equivalents when using the ``refresh`` query parameter. If ``async``, Elasticsearch will refresh target shards in the background. If ``sync``, Elasticsearch blocks bulk requests until the refresh is complete. If ``default``, Elasticsearch will use the default refresh behavior.
* ``refresh`` (optional): Control Elasticsearch refresh behavior for bulk requests via the ``refresh`` bulk API query parameter. Valid values are ``true``, ``wait_for``, and ``false``. Parameter values are specified as a string. If ``true``, Elasticsearch will refresh target shards in the background. If ``wait_for``, Elasticsearch blocks bulk requests until affected shards have been refreshed. If ``false``, Elasticsearch will use the default refresh behavior.

With multiple ``clients``, Rally will split each document using as many splits as there are ``clients``. This ensures that the bulk index operations are efficiently parallelized but has the drawback that the ingestion is not done in the order of each document. For example, if ``clients`` is set to 2, one client will index the document starting from the beginning, while the other will index starting from the middle.

Expand Down
22 changes: 10 additions & 12 deletions esrally/driver/runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,9 +492,9 @@ async def __call__(self, es, params):
* ``timeout``: a time unit value indicating the server-side timeout for the operation
* ``request-timeout``: a non-negative float indicating the client-side timeout for the operation. If not present, defaults to
``None`` and potentially falls back to the global timeout setting.
* ``refresh``: If ``async``, Elasticsearch will issue an async refresh to the index like ?refresh=true.
If ``sync``, Elasticsearch issues a synchronous refresh to the index like ?refresh=wait_for.
If ``default``, Elasticsearch will use refresh defaults like ?refresh=false.
* ``refresh``: If ``"true"``, Elasticsearch will issue an async refresh to the index; i.e., ``?refresh=true``.
If ``"wait_for"``, Elasticsearch issues a synchronous refresh to the index; i.e., ``?refresh=wait_for``.
If ``"false""``, Elasticsearch will use refresh defaults; i.e., ``?refresh=false``.
"""
detailed_results = params.get("detailed-results", False)
api_kwargs = self._default_kw_params(params)
Expand All @@ -505,16 +505,14 @@ async def __call__(self, es, params):
if "pipeline" in params:
bulk_params["pipeline"] = params["pipeline"]
if "refresh" in params:
refresh_params = {"sync": "wait_for", "async": "true", "default": "false"}
bulk_params["refresh"] = refresh_params.get(params["refresh"])

if bulk_params["refresh"] is None:
self.logger.warning(
"Using default bulk refresh parameter value '%s' in place of unrecognized specfified parameter value '%s'. "
"Use one of %s.",
refresh_params["default"],
valid_refresh_values = ("wait_for", "true", "false")
if params["refresh"] in valid_refresh_values:
bulk_params["refresh"] = params["refresh"]
else:
self.logger.info(
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for my last comment, I see now what you're checking for. I think both INFO and WARNING aren't enough this case, and we should probably just error out because of an invalid value - otherwise we risk returning misleading results and the corresponding log message is buried deep in the file somewhere.

Maybe we can error out similar to how it's done here?:

rally/esrally/driver/runner.py

Lines 2609 to 2612 in b1a822c

if op_type not in self.supported_op_types:
raise exceptions.RallyAssertionError(
f"Unsupported operation-type [{op_type}]. Use one of [{', '.join(self.supported_op_types)}]."
)

Copy link
Member

Choose a reason for hiding this comment

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

We could probably also just lean on the client itself to return an error, if you pass something in that's not valid does it return anything meaningful?

"Using default bulk refresh parameter value \"false\" in place of unrecognized parameter value '%s'. Use one of %s.",
params["refresh"],
", ".join(refresh_params.keys()),
", ".join(valid_refresh_values),
)

with_action_metadata = mandatory(params, "action-metadata-present", self)
Expand Down
8 changes: 4 additions & 4 deletions esrally/track/params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,10 +722,10 @@ def __init__(
:param recency: A number between [0.0, 1.0] indicating whether to bias generation of conflicting ids towards more recent ones.
May be None.
:param pipeline: The name of the ingest pipeline to run.
:param refresh: Optional values are "async", "sync", "default".
If "async", Elasticsearch refreshes the affected shards in the background.
If "sync", the client is blocked until Elasticsearch finishes the refresh operation.
If "default", Elasticsearch will use the default refresh behavior.
:param refresh: Optional string values are "true", "wait_for", "false".
If "true", Elasticsearch refreshes the affected shards in the background.
If "wait_for", the client is blocked until Elasticsearch finishes the refresh operation.
If "false", Elasticsearch will use the default refresh behavior.
:param original_params: The original dict passed to the parent parameter source.
"""
self.corpora = corpora
Expand Down
Loading