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

fix download from s3/gs by moving http query parameter downstream #1293

Merged
merged 7 commits into from
Jun 28, 2021

Conversation

hendrikmuhs
Copy link

@hendrikmuhs hendrikmuhs commented Jun 24, 2021

Fix a regression from #1159, the added no kpi parameter breaks downloads from s3/gs. The change adds a check of the protocol in add_url_param_elastic_no_kpi and only adds the parameter if its http

@hendrikmuhs hendrikmuhs added the bug Something's wrong label Jun 24, 2021
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thanks for spotting (and fixing) this! The code in the net module is used for downloads in general, such as downloading ES artifacts but also corpora. The logic of disabling KPI tracking makes sense for ES artifacts but not so much for the rest. May I therefore suggest that we really go with your other proposal of checking for the correct scheme before adding the request parameter? Here is a proposed change:

diff --git a/esrally/utils/net.py b/esrally/utils/net.py
index 4c0a868..7f2ad71 100644
--- a/esrally/utils/net.py
+++ b/esrally/utils/net.py
@@ -194,7 +194,11 @@ def download_http(url, local_path, expected_size_in_bytes=None, progress_indicat
         return expected_size_in_bytes

 def add_url_param_elastic_no_kpi(url):
-    return _add_url_param(url, {"x-elastic-no-kpi": "true"})
+    scheme = urllib3.util.parse_url(url).scheme
+    if scheme.startswith("http"):
+        return _add_url_param(url, {"x-elastic-no-kpi": "true"})
+    else:
+        return url

@danielmitterdorfer danielmitterdorfer added the :Benchmark Candidate Management Anything affecting how Rally sets up Elasticsearch label Jun 24, 2021
@danielmitterdorfer danielmitterdorfer added this to the 2.3.0 milestone Jun 24, 2021
@danielmitterdorfer danielmitterdorfer self-assigned this Jun 24, 2021
@hendrikmuhs
Copy link
Author

👍 I missed that this is also used for downloading data files.

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! LGTM. Let's wait for CI to finish, then I'll merge this.

@danielmitterdorfer danielmitterdorfer merged commit 44fda0c into elastic:master Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Benchmark Candidate Management Anything affecting how Rally sets up Elasticsearch bug Something's wrong
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants