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

Compress layers on push to a v2 registry #11869

Merged
merged 1 commit into from Mar 30, 2015

Conversation

dmcgowan
Copy link
Member

When buffering to file add support for compressing the tar contents. Since digest should be computed while writing buffer, include digest creation during buffer.

Pull auto detects compression requiring no additional changes.

Ping @jlhawn

@jlhawn
Copy link
Contributor

jlhawn commented Mar 27, 2015

LGTM

@jfrazelle Thanks for adding this to the 1.6.0 milestone! It is going to be very important for the release as it significantly reduces payload sizes which in turn reduces registry storage usage and bandwidth requirements.

Smaller downloads == Faster downloads == Happier users 👍

@jessfraz
Copy link
Contributor

ping @crosbymichael this is enhancements to make things faster if its
alright with you we should consider for 1.6

On Fri, Mar 27, 2015 at 2:03 PM, Josh Hawn notifications@github.com wrote:

LGTM

@jfrazelle https://github.com/jfrazelle Thanks for adding this to the
1.6.0 milestone! It is going to be very important for the release as it
significantly reduces payload sizes which in turn reduces registry storage
usage and bandwidth requirements.

Smaller downloads == Faster downloads == Happier users [image: 👍]


Reply to this email directly or view it on GitHub
#11869 (comment).

w = gzip.NewWriter(w)
}
_, err := io.Copy(w, src)
if wc, ok := w.(io.Closer); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this works, you need to close the underlying file and reset the hash manually. you cannot cast the multiwriter here.

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, i see that is going on now, if it's a gzip.Writer then the hash will not be populated, is this correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, sorry it is kind of confusing. Yes it is only for closing the gzip writer, the multiwriter does not need to be closed.

Copy link
Member

Choose a reason for hiding this comment

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

If it's confusing; perhaps some docs describing the function to prevent future coders tripping over it?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can be more explicit with the code:

func bufferToFile(f *os.File, src io.Reader, compress bool) (int64, digest.Digest, error) {
    var (
        d = digest.NewCanonicalDigester() // Uses sha256.
        w = io.MultiWriter(f, &d)
        gz *gzip.Writer
    )

    if compress {
        gz = gzip.NewWriter(w)
        w = gz
    }

    _, err := io.Copy(w, src)
    if gz != nil {
        gz.Close()
    }
    if err != nil {
        return 0, "", err
    }

    n, err := f.Seek(0, os.SEEK_CUR)
    if err != nil {
        return 0, "", err
    }

    if _, err := f.Seek(0, os.SEEK_SET); err != nil {
        return 0, "", err
    }

    return n, d.Digest(), nil
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment as @thaJeztah suggested, should be all that is required

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@dmcgowan dmcgowan force-pushed the v2-push-gzipped branch 2 times, most recently from e9a9a08 to 1692937 Compare March 28, 2015 00:09
@dmcgowan
Copy link
Member Author

Updated to make code easier to read based on @jlhawn's recommendation

@crosbymichael
Copy link
Contributor

Thanks for the readability changes. I'm getting rusty and cannot read code anymore.

@jlhawn
Copy link
Contributor

jlhawn commented Mar 28, 2015

@dmcgowan I was talking to @crosbymichael and @NathanMcCauley IRL about this PR and we agreed that if bufferToFile() isn't called from anywhere else in the project that we should just not have a compressed argument and just always compress.

@crosbymichael
Copy link
Contributor

@dmcgowan can you remove that bool?

@dmcgowan
Copy link
Member Author

Updated

@jlhawn
Copy link
Contributor

jlhawn commented Mar 30, 2015

LGTM

@crosbymichael
Copy link
Contributor

@jlhawn that LGTM is not parse-able

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 30, 2015

@dmcgowan Could you rebase against master and fix compilation errors?

When buffering to file add support for compressing the tar contents. Since digest should be computed while writing buffer, include digest creation during buffer.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
@crosbymichael
Copy link
Contributor

LGTM

crosbymichael added a commit that referenced this pull request Mar 30, 2015
Compress layers on push to a v2 registry
@crosbymichael crosbymichael merged commit 72dfd67 into moby:master Mar 30, 2015
@dmcgowan dmcgowan deleted the v2-push-gzipped branch March 30, 2015 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants