Add header enabling gzip downloads

[tartavull]

Not really intended to be merged.
I'm just wondering if there is any other way of downloading gzip files that has the correct content-encoding.

[dhermes]

@tartavull Can you give a code snippet that doesn't work the way you expect (and explain what you do expect)?

[tartavull]

Upload a compress file

 def _compress(content):
        stringio = StringIO()
        gzip_obj = gzip.GzipFile(mode='wb', fileobj=stringio)
        gzip_obj.write(content)
        gzip_obj.close()
        return stringio.getvalue()
content = 'highly compressible string'
blob.upload_from_string(compress(content))
blob.content_encoding = "gzip"
blob.patch()

Downloading file

assert content == blob.download_as_string()

I expect that when the http request is made it has the header accept-encoding:gzip
otherwise the server will decompress the blob and send something much larger.

[lukesneeringer]

Sending Accept-Encoding: gzip does not seem like a hack to me. That is the entire point of the Accept-Encoding header. I would be willing to accept this, so long as it does not have problematic side effects (e.g. causing trouble for other compression formats or uncompressed files).

[tartavull]

I haven't seen problems when retrieving uncompressed files, but I haven't test other compression formats.

[lukesneeringer]

@dhermes Any concerns about this PR? I have no problem adding an Accept-Encoding header.

[dhermes]

I'd like to see what @thobrla or someone else from the Storage team says. I'm just not sure if it makes sense.

[thobrla]

It's reasonable (even preferable) to include this header, but it is a substantial semantic change. How does the library handle gzipped bytes in the response? Is it up to the caller to decompress them?

[tartavull]

https://github.com/GoogleCloudPlatform/google-auth-library-python-httplib2 takes care of decompression, no action required by the caller

[dhermes]

As does urllib3 / requests

[thobrla]

Seems fine, then. The caller has always gotten uncompressed bytes and they'll continue to get uncompressed bytes.

Out of curiosity, how were mid-download connection breaks handled for content-encoding:gzip objects previously?

[tartavull]

I think you will now get an IOError: CRC check failed exception
from https://github.com/python-git/python/blob/master/Lib/gzip.py#L316

[lukesneeringer]

@tartavull Can you update the unit tests that fail as a result of your change? Once that is done, we can accept this.

[tartavull]

Do you happen to know how

Warning, treated as error:
/var/code/gcp/docs/logging-usage.rst.rst:25:Over dedent has detected
nox > Command bash ./test_utils/scripts/update_docs.sh failed with exit code 1
nox > Session docs failed. :(
Exited with code 1

is related to the commit changes?

[dhermes]

@tartavull Sorry for being quiet here for way too long.

• No the update_docs.sh failure is not your fault, it's related to the 1.6.1 release of Sphinx. Just rebase your branch on master and everything will be OK
• I'd really love to discussion discuss some snippets of code with the current implementation that doesn't work as you expect and then contrast that with how the same code runs with the change in this PR

[tartavull]

@dhermes looking forward to discuss them.

[dhermes]

@tartavull Do you have some examples? Or did you mean something else?

[tartavull]

I missed understand you, I don't have any snippets that this PR fixes, it is just a huge performance improvement, that's why I tagged the commit with "perf".
As shown in the snippet above, there are no changes in the output to the caller.

To provide you with a real use case, we store large 3d stacks of images in small 3d chunks.
These chunks are sized 512x512x64, and are of type uint64. Each uncompressed chunk is 128mb, but as you can see in the image they are mostly the same value so they compress to around 500kb.

[dhermes]

Ah I see. Let me play around a bit with this to try to "break things" / investigate the raw payloads.

In the meanwhile, you can check out the underlying library used for uploads (docs and source). You can pass in custom headers to an upload so this perf optimization would be usable immediately for you.

[tartavull]

Any luck breaking things?

[tartavull]

@dhermes Is there anything I can do to get this merged?

[dhermes]

Sorry @tartavull it fell off my plate of things to do! Really bad of me, eek.

I want to test this PR on real use cases before merging. In particular I'd like to test on two files:

• File A: Plain text
• File B: File A, but gzip-ed locally

and maybe some other cases I haven't thought of?

I just want to make sure this "does the right thing".

[tartavull]

@dhermes I understand. Let me know if you need any help from me.

[lukesneeringer]

Poke.

[tartavull]

This PR modifies a single line of code, and it reduces the traffic to google cloud storage for all your users downloading compressed files. For our particular case that's 250x reduction in bytes downloaded.
But after 90 days of created it hasn't being merged.

[lukesneeringer]

@dhermes At this point I want to just merge this. Giving you a chance to throw up a flag, but this should really get in.

[dhermes]

@lukesneeringer If you feel it's the right move, go for it.

I worry that it is partially incorrect, e.g. of the thing stored is gzipped then it will accidentally do the wrong thing.

[lukesneeringer]

I am not particularly worried. This is a really common pattern.

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[tartavull]

Thanks :)