Feature/range bytes #2890

quasiben · 2016-12-20T19:03:03Z

PR attempts to resolve #2888. Users should be able to easily download a range of bytes when working with storage blobs. This PR tries to minimally inject a new kwarg: range_bytes in download_as_string and related downloading functions:

# download the first 100 bytes
blob.download_as_string(range_bytes=(0,100))

# download the second 100 bytes
blob.download_as_string(range_bytes=(99,200))

cc @daspecster

googlebot · 2016-12-20T19:03:05Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

daspecster · 2016-12-20T19:19:25Z

@quasiben awesome! Thanks!

I'll start a review on this right now.

daspecster

Looks really good!

core/google/cloud/streaming/transfer.py

@@ -370,18 +370,30 @@ def initialize_download(self, http_request, http):

        :type http: :class:`httplib2.Http` (or workalike)
        :param http: Http instance for this request.
+
+        :type range_bytes: tuple
+        :param range_bytes: Optional. Range of bytes to download.


core/google/cloud/streaming/transfer.py

-            self._set_range_header(http_request, 0, end_byte)
+            if range_bytes:
+                start_byte, end_byte = range_bytes
+                end_byte = end_byte - 1


core/google/cloud/streaming/transfer.py

            response = make_api_request(
                self.bytes_http or http, http_request)
            if response.status_code not in self._ACCEPTABLE_STATUSES:
                raise HttpError.from_response(response)
            self._initial_response = response
            self._set_total(response.info)
+            # when using `range_bytes` we need to reset total_size


storage/google/cloud/storage/blob.py

@@ -319,6 +319,9 @@ def download_to_file(self, file_obj, client=None):
        :param client: Optional. The client to use.  If not passed, falls back
                       to the ``client`` stored on the blob's bucket.

+        :type range_bytes: tuple
+        :param range_bytes: Optional. Range of bytes to download.


storage/google/cloud/storage/blob.py

        """Download the contents of this blob as a string.

        :type client: :class:`~google.cloud.storage.client.Client` or
                      ``NoneType``
        :param client: Optional. The client to use.  If not passed, falls back
                       to the ``client`` stored on the blob's bucket.

+        :type range_bytes: tuple
+        :param range_bytes: Optional. Range of bytes to download.


daspecster · 2016-12-20T19:32:58Z

Aside from my little nitpicks, if you want to know how to run the tests locally, I recommend running a few things and making sure they pass before pushing up.

tox -e py27 -- core storage
tox -e py35 -- core storage
tox -e cover -- core storage
tox -e lint

A guide to setting up tox and running these can be found in our CONTRIBUTING.rst.

quasiben · 2016-12-20T22:35:48Z

Thanks for the review @daspecster

quasiben · 2016-12-20T22:45:14Z

I'll have to get another setup to properly use tox. Currently, my main Python environments are conda based

dhermes

Before I get deeper in this review, I want to point something out.

As part of "divorcing" httplib2 we (@jonparrott and I) plan on greatly re-factoring google.cloud.streaming.

core/google/cloud/streaming/transfer.py

+        :param range_bytes: (Optional). Range of bytes to download.
+
+        For more info on Range Bytes:
+        https://cloud.google.com/storage/docs/xml-api/reference-headers#range


core/google/cloud/streaming/transfer.py

        """
        self._ensure_uninitialized()
        url = http_request.url
        if self.auto_transfer:
-            end_byte = self._compute_end_byte(0)
-            self._set_range_header(http_request, 0, end_byte)
+            if range_bytes:


core/google/cloud/streaming/transfer.py

+            if range_bytes:
+                start_byte, end_byte = range_bytes
+                start_byte = int(start_byte)
+                end_byte = int(end_byte)


core/google/cloud/streaming/transfer.py

+            # when using `range_bytes` we need to reset total_size,
+            # otherwise we get the total of the file and the entire contents
+            # are downloaded
+            if range_bytes: self._total_size = abs(end_byte - start_byte)


quasiben · 2016-12-21T02:40:27Z

@dhermes happy to wait on this work if the "divorce" is imminent

quasiben · 2016-12-21T04:17:56Z

I signed it!

googlebot · 2016-12-21T04:17:57Z

CLAs look good, thanks!

quasiben · 2016-12-22T17:50:31Z

anything else that needs doing here?

daspecster · 2016-12-22T18:22:48Z

@quasiben your changes that I requested look good!

I think @dhermes is going to look into this more based on his comment so let's see what suggestions he has before merging.

quasiben · 2016-12-22T18:30:32Z

Sounds good

daspecster · 2017-01-05T14:19:16Z

@dhermes did you have any other feedback for this?

daspecster · 2017-01-09T15:54:48Z

@dhermes just a reminder about this.

daspecster · 2017-01-30T21:15:51Z

daspecster · 2017-01-30T21:30:14Z

Thank you so much for this @quasiben! However after talking to @dhermes, it sounds like a better path may be to work towards #1998 and remove httplib2.

I'm going to close this for now and try and work towards #1998.

quasiben · 2017-01-30T21:35:35Z

Ok, any ideas on a timeline for #1998 ?

daspecster · 2017-01-30T22:22:27Z

@dhermes has a plan in mind I think.

@dhermes is there anyway that I can help with this after this week(due to the other stuff we have to get done)?

quasiben added 2 commits December 20, 2016 13:45

add range_bytes functionality to blob downloads

343ba37

add test for range_bytes

4b086de

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Dec 20, 2016

daspecster suggested changes Dec 20, 2016

View reviewed changes

fix args in initialize_download and some cleanup

d2b790b

quasiben force-pushed the feature/range_bytes branch from 09002c4 to d2b790b Compare December 20, 2016 22:48

dhermes reviewed Dec 21, 2016

View reviewed changes

quasiben added 3 commits December 20, 2016 21:06

linting

a39fe65

fix up total_size calculation, fix test_blob, fix doc string

0680047

add new range_bytes test to test_transfer

36cd637

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 21, 2016

daspecster approved these changes Dec 22, 2016

View reviewed changes

daspecster added api: core api: storage Issues related to the Cloud Storage API. labels Dec 28, 2016

daspecster requested a review from lukesneeringer January 30, 2017 21:20

daspecster requested a review from tseaver January 30, 2017 21:20

daspecster closed this Jan 30, 2017

danoscarmike mentioned this pull request Aug 9, 2017

How To Set Range #2888

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/range bytes #2890

Feature/range bytes #2890

quasiben commented Dec 20, 2016

googlebot commented Dec 20, 2016

daspecster commented Dec 20, 2016

daspecster left a comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

daspecster commented Dec 20, 2016

quasiben commented Dec 20, 2016

quasiben commented Dec 20, 2016

dhermes left a comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

quasiben commented Dec 21, 2016

quasiben commented Dec 21, 2016

googlebot commented Dec 21, 2016

quasiben commented Dec 22, 2016

daspecster commented Dec 22, 2016

quasiben commented Dec 22, 2016

daspecster commented Jan 5, 2017

daspecster commented Jan 9, 2017

daspecster commented Jan 30, 2017

daspecster commented Jan 30, 2017

quasiben commented Jan 30, 2017

daspecster commented Jan 30, 2017

Feature/range bytes #2890

Feature/range bytes #2890

Conversation

quasiben commented Dec 20, 2016

googlebot commented Dec 20, 2016

daspecster commented Dec 20, 2016

daspecster left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

daspecster commented Dec 20, 2016

quasiben commented Dec 20, 2016

quasiben commented Dec 20, 2016

dhermes left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

quasiben commented Dec 21, 2016

quasiben commented Dec 21, 2016

googlebot commented Dec 21, 2016

quasiben commented Dec 22, 2016

daspecster commented Dec 22, 2016

quasiben commented Dec 22, 2016

daspecster commented Jan 5, 2017

daspecster commented Jan 9, 2017

daspecster commented Jan 30, 2017

daspecster commented Jan 30, 2017

quasiben commented Jan 30, 2017

daspecster commented Jan 30, 2017