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

Set enable_cleanup_closed by default #1469

Merged
merged 3 commits into from
Mar 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions docs/command_line_reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ Selects the track repository that Rally should use to resolve tracks. By default
``track-revision``
~~~~~~~~~~~~~~~~~~

Selects a specific revision in the track repository. By default, Rally will choose the most appropriate branch on its own but in some cases it is necessary to specify a certain commit. This is mostly needed when testing whether a change in performance has occurred due to a change in the workload.
Selects a specific revision in the track repository. By default, Rally will choose the most appropriate branch on its own but in some cases it is necessary to specify a certain commit. This is mostly needed when testing whether a change in performance has occurred due to a change in the workload.

``track``
~~~~~~~~~
Expand Down Expand Up @@ -656,7 +656,7 @@ Default value: ``timeout:60`` (applies any time ``timeout`` is not specified)
Rally recognizes the following client options in addition:

* ``max_connections``: By default, Rally will choose the maximum allowed number of connections automatically (equal to the number of simulated clients but at least 256 connections). With this property it is possible to override that logic but a minimum of 256 is enforced internally.
* ``enable_cleanup_closed`` (default: ``false``): In some cases, SSL connections might not be properly closed and the number of open connections increases as a result. When this client option is set to ``true``, the Elasticsearch client will check and forcefully close these connections.
* ``enable_cleanup_closed`` (default: ``true``): In some cases, `Elasticsearch does not properly close SSL connections <https://github.com/elastic/elasticsearch/issues/76642>`_ and the number of open connections increases as a result. When this client option is set to ``true``, the Elasticsearch client will check and forcefully close these connections.
* ``static_responses``: The path to a JSON file containing path patterns and the corresponding responses. When this value is set to ``true``, Rally will not send requests to Elasticsearch but return static responses as specified by the file. This is useful to diagnose performance issues in Rally itself. See below for a specific example.

**Examples**
Expand Down
2 changes: 1 addition & 1 deletion esrally/async_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ def __init__(
)

self._trace_configs = [trace_config] if trace_config else None
self._enable_cleanup_closed = kwargs.get("enable_cleanup_closed", False)
self._enable_cleanup_closed = kwargs.get("enable_cleanup_closed", True)

static_responses = kwargs.get("static_responses")
self.use_static_responses = static_responses is not None
Expand Down
13 changes: 13 additions & 0 deletions tests/client_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import urllib3.exceptions

from esrally import client, doc_link, exceptions
from esrally.async_connection import AIOHttpConnection
from esrally.utils import console
from tests import run_async

Expand Down Expand Up @@ -349,3 +350,15 @@ def test_ssl_error(self, es):
)
with pytest.raises(exceptions.SystemSetupError, match="Could not connect to cluster via https. Is this an https endpoint?"):
client.wait_for_rest_layer(es, max_attempts=3)


class TestAsyncConnection:
@run_async
async def test_enable_cleanup_close(self):
connection = AIOHttpConnection()
# pylint: disable=protected-access
assert connection._enable_cleanup_closed is True

connection = AIOHttpConnection(enable_cleanup_closed=False)
# pylint: disable=protected-access
assert connection._enable_cleanup_closed is False
6 changes: 3 additions & 3 deletions tests/driver/runner_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -5795,13 +5795,13 @@ async def test_adds_request_timings(self):
assert len(timings) == 3

assert timings[0]["operation"] == "initial-call"
assert math.isclose(timings[0]["service_time"], 0.1, rel_tol=0.05)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you suppose the change in enable_cleanup_closed somehow affected the RequestContextHolder's APIs' responsiveness? I don't have an issue in general with adding tolerance to approximation unit tests, but wondering if we're going to expect anything else to happen in service_times even for short-lived benchmarks.

For context, I haven't seen this test fail before

Copy link
Member Author

Choose a reason for hiding this comment

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

No, RequestContextHolder is purely self-contained. I think it's just that when moving to pytest I made the test stricter.

assert timings[0]["service_time"] == pytest.approx(0.1, abs=0.1)

assert timings[1]["operation"] == "stream-a"
assert math.isclose(timings[1]["service_time"], 0.2, rel_tol=0.05)
assert timings[1]["service_time"] == pytest.approx(0.2, abs=0.1)

assert timings[2]["operation"] == "stream-b"
assert math.isclose(timings[2]["service_time"], 0.1, rel_tol=0.05)
assert timings[2]["service_time"] == pytest.approx(0.1, abs=0.1)

# common properties
for timing in timings:
Expand Down