-
Notifications
You must be signed in to change notification settings - Fork 302
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
support suggest.buildAll and add a plugin to execute queries without waiting for Solr's response #1119
Conversation
added NoResponseRequest plugin
@thomascorthals documentation needs to be added if accepted |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1119 +/- ##
==========================================
- Coverage 97.75% 97.74% -0.01%
==========================================
Files 399 400 +1
Lines 10467 10520 +53
==========================================
+ Hits 10232 10283 +51
- Misses 235 237 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the use of TimeoutAwareInterface::MINIMUM_TIMEOUT
.
From https://curl.se/libcurl/c/CURLOPT_TIMEOUT.html:
With CURLOPT_CONNECTTIMEOUT set to 4 and CURLOPT_TIMEOUT set to 2, the operation can never last longer than 2 seconds.
If the adapter is also ConnectionTimeoutAware, the plugin should add that timeout to MINIMUM_TIMEOUT
I think.
@thomascorthals thanks for your review. I modified the code and added some minimal documentation. |
It still doesn't take the connection timeout into account in the plugin. I wrote this example (I'll commit the cleaned up version afterwards). <?php
require_once __DIR__.'/init.php';
htmlHeader();
// set a long connection timeout
$adapter->setConnectionTimeout(10);
// connect to a host that will never complete the connection
$config['endpoint']['localhost']['host'] = '10.0.0.0';
// create a client instance
$client = new Solarium\Client($adapter, $eventDispatcher, $config);
// get a suggester query instance and add setting to build all suggesters
$suggester = $client->createSuggester();
$suggester->setBuildAll(true);
// don't wait unitl all suggesters have been built
$client->getPlugin('nowaitforresponserequest');
// this executes the query
$client->suggester($suggester);
htmlFooter(); When I comment out the plugin, this results in:
With the plugin, this script ends after 1 second. If I
That should also be slightly above 10000 milliseconds. And if I run it on
|
$timeout = $this->client->getAdapter()->getTimeout(); | ||
$fastTimeout = TimeoutAwareInterface::FAST_TIMEOUT; | ||
if ($this->client->getAdapter() instanceof ConnectionTimeoutAwareInterface) { | ||
$fastTimeout += $this->client->getAdapter()->getConnectionTimeout() ?? 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hinges on the assumption that all ConnectionTimeoutAware adapters will behave like Curl in this regard.
@mkalkbrenner Actually, we only want to return a |
… into buildAll_noresponse
… into buildAll_noresponse
@thomascorthals I just implemented that right before you posted this comment :-) |
@thomascorthals I assume it is finished now. |
… into buildAll_noresponse
Now the failing tests proven that exceptions ar thrown ;-) |
@thomascorthals Now that the test are passing, should we merge this one and got for a release to fix the documentation? |
@mkalkbrenner Go for it! |
We noticed that building suggesters can be very expensive.
PHP scripts or their HTTP connections to Solr might run into timeouts on large indexes.
The NoWaitForResponseRequest plugin introduces a kind of fire-and-forget queries.