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 issue with fetching test-mode -1k document corpora. #885

Merged
merged 3 commits into from
Feb 3, 2020

Conversation

drawlerr
Copy link
Contributor

In test-mode, we download a document corpus with "-1k" added to the filename, that is not specifically described in track.json. This is problematic because, the "expected bytes" of the file is None in this situation, breaking download progress reporting when we divide by None.
To work around this, I've made the net.Progress callback resilient to this situation and added a test.

@drawlerr drawlerr added the bug Something's wrong label Jan 31, 2020
@drawlerr drawlerr added this to the 1.4.1 milestone Jan 31, 2020
@drawlerr drawlerr self-assigned this Jan 31, 2020
@danielmitterdorfer danielmitterdorfer added the :Track Management New operations, changes in the track format, track download changes and the like label Jan 31, 2020
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.

I have to admit I was puzzled at first why this does not happen all the time but I understand now why we (usually) don't have this issue: this only happens if we can't derive the expected size via other means. An alternative (or amendment) to this PR could be to ensure we determine the expected size also for S3 downloads (worked fine in a local test):

diff --git a/esrally/utils/net.py b/esrally/utils/net.py
index 5ae6168..445bd9b 100644
--- a/esrally/utils/net.py
+++ b/esrally/utils/net.py
@@ -82,10 +82,13 @@ def _download_from_s3_bucket(bucket_name, bucket_path, local_path, expected_size
             self._progress(self._bytes_read, self._expected_size_in_bytes)

     s3 = boto3.resource("s3")
+    bucket = s3.Bucket(bucket_name)
+    if expected_size_in_bytes is None:
+        expected_size_in_bytes = bucket.Object(bucket_path).content_length
     progress_callback = S3ProgressAdapter(expected_size_in_bytes, progress_indicator) if progress_indicator else None
-    s3.Bucket(bucket_name).download_file(bucket_path, local_path,
-                                         Callback=progress_callback,
-                                         Config=boto3.s3.transfer.TransferConfig(use_threads=False))
+    bucket.download_file(bucket_path, local_path,
+                         Callback=progress_callback,
+                         Config=boto3.s3.transfer.TransferConfig(use_threads=False))

What do you think?

tests/utils/net_test.py Outdated Show resolved Hide resolved
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 iterating! LGTM

@drawlerr drawlerr merged commit 1505223 into elastic:master Feb 3, 2020
@drawlerr drawlerr deleted the test-mode-progress-error branch February 3, 2020 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something's wrong :Track Management New operations, changes in the track format, track download changes and the like
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants