Skip to content

Addition of gzip feature #568

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

Open
wants to merge 3 commits into
base: v3
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"ext-curl": "*",
"ext-json": "*",
"ext-mbstring": "*",
"ext-zlib": "^7.3",
"psr/http-message": "^1.0",
"psr/log": "^1.0",
"psr/simple-cache": "^1.0"
Expand Down
25 changes: 25 additions & 0 deletions src/Config/AbstractConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public function getDefaultConfig()
'writeTimeout' => $this->defaultWriteTimeout,
'connectTimeout' => $this->defaultConnectTimeout,
'defaultHeaders' => array(),
'gzipEnabled' => false,
);
}

Expand Down Expand Up @@ -115,4 +116,28 @@ public function setDefaultHeaders(array $defaultHeaders)

return $this;
}

/**
* @return boolean
*/
public function getGzipEnabled()
{
return $this->config['gzipEnabled'];
}

/**
* @param boolean $gzipEnabled
*
* @return $this
*/
public function setGzipEnabled($gzipEnabled)
{
if (!is_bool($gzipEnabled)) {
throw new \InvalidArgumentException('Default configuration for GzipEnabled must be a boolean');
}

$this->config['gzipEnabled'] = $gzipEnabled;

return $this;
}
}
1 change: 1 addition & 0 deletions src/Config/SearchConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -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?

);
}

Expand Down
4 changes: 4 additions & 0 deletions src/Http/Psr7/Request.php
Original file line number Diff line number Diff line change
Expand Up @@ -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

}

if ('' !== $body && null !== $body) {
$this->stream = stream_for($body);
}
Expand Down
18 changes: 18 additions & 0 deletions src/RetryStrategy/ApiWrapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}

$uri = $this->createUri($path)
->withQuery($requestOptions->getBuiltQueryParameters())
->withScheme('https');
Expand Down Expand Up @@ -215,6 +219,14 @@ private function createUri($uri)
throw new \InvalidArgumentException('URI must be a string or UriInterface');
}

/**
* @param $method
* @param $uri
* @param array $headers
* @param null $body
* @param string $protocolVersion
* @return Request
*/
private function createRequest(
$method,
$uri,
Expand All @@ -239,6 +251,12 @@ private function createRequest(
return new Request($method, $uri, $headers, $body, $protocolVersion);
}

private function canEnableGzipCompress($method)
{
return (strtoupper($method) === 'POST' || strtoupper($method) === 'PUT')
&& $this->config->getGzipEnabled();
}

/**
* @param string $level
* @param string $message
Expand Down
1 change: 0 additions & 1 deletion tests/Integration/IndexingTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ public function testIndexing()
$multiResponse->wait();

/* Check 6 first records with getObject */

$objectID1 = $responses[0][0]['objectIDs'][0];
$objectID2 = $responses[1][0]['objectIDs'][0];
$objectID3 = $responses[2][0]['objectIDs'][0];
Expand Down