solariumphp / solarium

PHP Solr client library
Other
932 stars 301 forks source link

support suggest.buildAll and add a plugin to execute queries without waiting for Solr's response #1119

Closed mkalkbrenner closed 9 months ago

mkalkbrenner commented 10 months ago

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.

mkalkbrenner commented 10 months ago

@thomascorthals documentation needs to be added if accepted

codecov[bot] commented 10 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (557c38f) 97.75% compared to head (058006a) 97.74%. Report is 1 commits behind head on master.

Files Patch % Lines
src/Plugin/NoWaitForResponseRequest.php 95.55% 2 Missing :warning:
Additional details and impacted files ```diff @@ 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 ``` | [Flag](https://app.codecov.io/gh/solariumphp/solarium/pull/1119/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=solariumphp) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/solariumphp/solarium/pull/1119/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=solariumphp) | `97.74% <96.42%> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=solariumphp#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mkalkbrenner commented 10 months ago

@thomascorthals thanks for your review. I modified the code and added some minimal documentation.

thomascorthals commented 10 months ago

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:

Solr HTTP error: HTTP request failed, Connection timeout after 10005 ms

With the plugin, this script ends after 1 second. If I echo $e->getMessage() in the plugin's catch block, I get:

Solr HTTP error: HTTP request failed, Connection timed out after 1004 milliseconds

That should also be slightly above 10000 milliseconds.

And if I run it on localhost against ncat -k -l 8983 (a listener that never responds), that should actually be an operation (not a connection) timeout slightly above 11000 milliseconds.

Solr HTTP error: HTTP request failed, Operation timed out after 11013 milliseconds with 0 bytes received

thomascorthals commented 10 months ago

@mkalkbrenner Actually, we only want to return a HTTP/1.0 200 OK response for an operation timeout! A connection timeout should still throw an exception because in that case the request never went through and there is no way of telling with the current plugin implementation.

mkalkbrenner commented 10 months ago

@mkalkbrenner Actually, we only want to return a HTTP/1.0 200 OK response for an operation timeout! A connection timeout should still throw an exception because in that case the request never went through and there is no way of telling with the current plugin implementation.

@thomascorthals I just implemented that right before you posted this comment :-) But I had to resolve the conflicts first.

mkalkbrenner commented 10 months ago

@thomascorthals I assume it is finished now.

mkalkbrenner commented 10 months ago

Now the failing tests proven that exceptions ar thrown ;-)

mkalkbrenner commented 10 months ago

@thomascorthals Now that the test are passing, should we merge this one and got for a release to fix the documentation?

thomascorthals commented 10 months ago

@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!