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

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

Merged
merged 34 commits into from
Jan 10, 2024

Conversation

mkalkbrenner
Copy link
Member

@mkalkbrenner mkalkbrenner commented Jan 3, 2024

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
Copy link
Member Author

@thomascorthals documentation needs to be added if accepted

Copy link

codecov bot commented Jan 3, 2024

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 ⚠️
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     
Flag Coverage Δ
unittests 97.74% <96.42%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@thomascorthals thomascorthals left a 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.

tests/Integration/AbstractTechproductsTestCase.php Outdated Show resolved Hide resolved
src/Core/Client/Adapter/TimeoutAwareInterface.php Outdated Show resolved Hide resolved
src/Plugin/NoWaitForResponseRequest.php Show resolved Hide resolved
@mkalkbrenner
Copy link
Member Author

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

@thomascorthals
Copy link
Member

thomascorthals commented Jan 5, 2024

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

$timeout = $this->client->getAdapter()->getTimeout();
$fastTimeout = TimeoutAwareInterface::FAST_TIMEOUT;
if ($this->client->getAdapter() instanceof ConnectionTimeoutAwareInterface) {
$fastTimeout += $this->client->getAdapter()->getConnectionTimeout() ?? 0;
Copy link
Member

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.

@thomascorthals thomascorthals changed the title support sugest.buildAll and add a plugin to execute queries without waiting for Solr's response support suggest.buildAll and add a plugin to execute queries without waiting for Solr's response Jan 5, 2024
@thomascorthals
Copy link
Member

@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
Copy link
Member Author

@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
Copy link
Member Author

@thomascorthals I assume it is finished now.

@mkalkbrenner
Copy link
Member Author

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

@mkalkbrenner
Copy link
Member Author

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

@thomascorthals
Copy link
Member

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

thomascorthals
thomascorthals previously approved these changes Jan 5, 2024
@mkalkbrenner mkalkbrenner merged commit 913c00b into master Jan 10, 2024
52 of 54 checks passed
@mkalkbrenner mkalkbrenner deleted the buildAll_noresponse branch January 10, 2024 08:27
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.

2 participants