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

put *gzip.Writer back to pool #1441

Merged
merged 4 commits into from
Aug 14, 2017
Merged

put *gzip.Writer back to pool #1441

merged 4 commits into from
Aug 14, 2017

Conversation

Zeymo
Copy link
Contributor

@Zeymo Zeymo commented Aug 14, 2017

fixes #1439

@Zeymo Zeymo changed the title put z back to pool put *gzip.Writer back to pool Aug 14, 2017
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Can you also see why gofmt is failing? (Is it because of the item I commented on?)

rpc_util.go Outdated
@@ -63,6 +63,9 @@ func NewGZIPCompressor() Compressor {

func (c *gzipCompressor) Do(w io.Writer, p []byte) error {
z := c.pool.Get().(*gzip.Writer)
defer func(){
Copy link
Member

Choose a reason for hiding this comment

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

defer c.pool.Put(z)

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

This reverts commit ae489c4
@dfawley
Copy link
Member

dfawley commented Aug 14, 2017

Thanks for finding this and for the fix!

It looks like the second commit was done by xuechen.wzm -- if you redo this commit with your other username and git push -f, I believe that should take care of it.

@Zeymo
Copy link
Contributor Author

Zeymo commented Aug 14, 2017

sry for my careless : (

@dfawley
Copy link
Member

dfawley commented Aug 14, 2017

I'm not sure if this will work. I thought that since there was a commit by another author, it would say you hadn't signed the CLAs. But it's not complaining at the moment.

I'll be squashing and merging anyway, and since both authors are you, LGTM and let's hope it actually works. Thanks again for the fix.

@dfawley dfawley merged commit 1308414 into grpc:master Aug 14, 2017
@menghanl menghanl added 1.6 Type: Performance Performance improvements (CPU, network, memory, etc) labels Aug 24, 2017
@dfawley dfawley modified the milestone: 1.6 Release Aug 28, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Performance Performance improvements (CPU, network, memory, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gzip.Writer not return to Pool?
4 participants