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

Add support for gzip encoding in HTTP requests #50

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

vkuznet
Copy link
Contributor

@vkuznet vkuznet commented Nov 12, 2021

No description provided.

@vkuznet vkuznet requested a review from yuyiguo November 12, 2021 16:28
@vkuznet
Copy link
Contributor Author

vkuznet commented Nov 12, 2021

@yuyiguo , here is set of changes to allow DBSClient to use gzip encoding for HTTP POST requests, e.g. bulkblocks API. My original benchmark shows that usage of such encoding can save us 10-20 times on network, e.g. 180MB payload can be easily gzipped to 3MB before sending to server. Of course, the server itself should be aware of such encoding and handle properly such data. That's why I disable it by default (see useGzip=False in DbsApi constructor). But these changes provide a way to start using this encoding of data.

@amaltaro you can also use this as an example for other WMCore client's calls. And, it is related to dmwm/DBS#648 and dmwm/WMCore#10451 issues. I provided concise example of Cherrypy server implementation in my gist.

Copy link
Member

@yuyiguo yuyiguo left a comment

Choose a reason for hiding this comment

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

This gzip looks good to me. I think it is good to keep the default is off. This may only useful for the millions of lumi insertion. The network seems not a big consumer for DBS.

@vkuznet
Copy link
Contributor Author

vkuznet commented Nov 12, 2021

@yuyiguo it is a issue for our frontends though, if we can reduce payload on FE then FE will need less resources and process it faster. So, if you don't mind I'll merge it after I'll perform additional tests on BE servver.

@yuyiguo
Copy link
Member

yuyiguo commented Nov 12, 2021

@vkuznet
I don't mind to merge it at all.

Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

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

Valentin, besides the comment left along the code, I would suggest to always test both native python3 string and bytes type when implementing these string handling methods.

src/python/dbs/apis/dbsClient.py Outdated Show resolved Hide resolved
@vkuznet
Copy link
Contributor Author

vkuznet commented Nov 16, 2021

@amaltaro it would be nice if you'll review it before you're going to vacation, such that I can merge and proceed.

Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks Valentin

@vkuznet vkuznet merged commit ced79aa into dmwm:main Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants