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

Addition of gzip feature #568

Open
wants to merge 3 commits into
base: v3
Choose a base branch
from
Open

Addition of gzip feature #568

wants to merge 3 commits into from

Conversation

chloelbn
Copy link

@chloelbn chloelbn commented Jul 8, 2019

Q A
Bug fix? yes/no
New feature? yes/no
BC breaks? no
Related Issue Fix #563
Need Doc update yes/no

Describe your change

What problem is this fixing?

@chloelbn chloelbn changed the title Addition of zip feature [WIP] Addition of zip feature Jul 8, 2019
Copy link

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

Thanks for this pull request, good job.

Let's make sure we address my feedback before merging.

composer.json Outdated Show resolved Hide resolved
@@ -29,6 +29,7 @@ public function getDefaultConfig()
'defaultHeaders' => array(),
'defaultForwardToReplicas' => null,
'batchSize' => 1000,
'gzipEnabled' => true,

Choose a reason for hiding this comment

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

We really want to do this by default? @Ant-hem @aseure

Copy link
Member

@Ant-hem Ant-hem Jul 8, 2019

Choose a reason for hiding this comment

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

Yes we are going default for SearchClient but not for Analytics, Insights and Places (not supported yet).

However we discussed using an enum instead of a bool in case the engine introduces new compression type. See Java's PR and C#'s PR.

Choose a reason for hiding this comment

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

This is a good point, @chloelbn you may need to refactor the pull request in order to prepare the PHP client for other compression types.

Choose a reason for hiding this comment

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

Don't hesitate on syncing with me for tackling this refactorization.

Copy link
Author

Choose a reason for hiding this comment

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

done in the last commit, what are your thoughts?

@@ -62,6 +62,10 @@ public function __construct(
$this->updateHostFromUri();
}

if ($this->hasHeader('Content-Encoding')) {
$body = gzencode($body, 9);

Choose a reason for hiding this comment

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

Let's double check the level here, we are not sure if 9 is the correct level.

Choose a reason for hiding this comment

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

Let's unit test this to make sure we have this header in both requests, maybe is already the case.

Copy link
Author

Choose a reason for hiding this comment

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

tests added, as for compression 9 is the higher level but I guess it will take more time. Not sure how to benchmark which level to use

src/RetryStrategy/ApiWrapper.php Outdated Show resolved Hide resolved
src/RetryStrategy/ApiWrapper.php Show resolved Hide resolved
@@ -111,6 +111,10 @@ public function send($method, $path, $requestOptions = array(), $hosts = null)

private function request($method, $path, RequestOptions $requestOptions, $hosts, $timeout, $data = array())
{
if ($this->canEnableGzipCompress($method)) {
$requestOptions->addHeader('Content-Encoding', 'gzip');
Copy link
Member

@Ant-hem Ant-hem Jul 8, 2019

Choose a reason for hiding this comment

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

Is the underlying HTTP library computing the content-length header as well?

Copy link
Member

Choose a reason for hiding this comment

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

@chloelbn Seems like tests are still failing, is the underlying http library setting this header?
"Content-Type: application/json; charset=utf-8"

Copy link
Member

@Ant-hem Ant-hem Jul 10, 2019

Choose a reason for hiding this comment

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

Or, the gzip feature might not be enable on the test servers we are targeting with the PHP library. Are they the same at the CTS one @nunomaduro? I couldn't check on travis as they are ciphered.

@nunomaduro
Copy link

@chloelbn Let me know when I can review this 👍

@chloelbn chloelbn changed the title [WIP] Addition of zip feature Addition of gzip feature Jul 11, 2019
@nunomaduro
Copy link

@chloelbn I will check why travis is not passing the tests.

@nunomaduro
Copy link

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.

GZIP on POST/PUT requests
3 participants