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

It's me again :) http.compression more effective ! #932

Merged
merged 2 commits into from
Dec 13, 2015
Merged

It's me again :) http.compression more effective ! #932

merged 2 commits into from
Dec 13, 2015

Conversation

yvann
Copy link

@yvann yvann commented Sep 16, 2015

Moved the "accept-encoding" part to be always effective, the compression will depend of the ES configuration, we only say that we support it.

@ruflin
Copy link
Owner

ruflin commented Sep 17, 2015

@yvann Interesting. Can you please add at least one integration test with compression enabled so we can make sure this works as expected?

@yvann
Copy link
Author

yvann commented Sep 17, 2015

Sure ! I just saw the last comment you did on my previous PR :)

@ruflin
Copy link
Owner

ruflin commented Oct 18, 2015

@yvann Any updates here?

@ruflin
Copy link
Owner

ruflin commented Dec 9, 2015

@yvann I would love to get this into master. Any updates here?

@yvann
Copy link
Author

yvann commented Dec 10, 2015

I have no good excuse to not do it, let me the day to push something :) !

@ruflin
Copy link
Owner

ruflin commented Dec 10, 2015

@yvann 👍 :-D

@yvann
Copy link
Author

yvann commented Dec 12, 2015

Rebased and added some tests, at least to show that the compression parameter doesn't affect the success of requests.
The behavior for the Http transport is :

  • Elastica always says to ES it supports compression (via curl native support), to allow ES to send back a compressed response
  • Depending of compression parameter, the request sent to ES is compressed or not

ruflin added a commit that referenced this pull request Dec 13, 2015
It's me again :) http.compression more effective !
@ruflin ruflin merged commit c000d5d into ruflin:master Dec 13, 2015
@ruflin
Copy link
Owner

ruflin commented Dec 13, 2015

Merged, thx

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