drupal-graphql / graphql-search-api

GraphQL & Drupal Search API integration.
10 stars 22 forks source link

Add configurable cache max age to GraphQL Search API query results #48

Closed bronius closed 3 years ago

bronius commented 3 years ago

Caching is vital -- but when working with a very large or otherwise busy Search API index, the Search API tracking can report a successfully indexed document to GraphQL Search API while it's not actually been indexed in solr (or whatever backend) yet before the client has made the query following having entered new records.

In our application, we found it helpful to artificially reduce cache max-age to allow more opportunity for the indexer to present the freshest data as soon as it is available.

I am working on a patch for this now.

bronius commented 3 years ago

GraphQL_Search_API_Settings_form I am not sure I'm pushing a patch proposal / PR correctly here :) The above screenshot is what my commit provides. The configured value is applied in SearchAPISearch::getCacheDependencies like:

     $metadata = new CacheableMetadata();

+    $metadata->setCacheMaxAge($this->settings->get('cache_max_age'));
     $metadata->addCacheTags(['search_api_list:' . $this->index->id()]);

I failed to include cache invalidation on submit of the config form: It would be good to clear cache for the search_api_list:* affected whenever its cache max-age is adjusted.

duartegarin commented 3 years ago

Hi @bronius , Thank you for submitting this. While what you are saying is true (and in fact we use a similar thing internally to set max age headers) I don't think this belongs in the GraphQL Search API module. The scope of responsibility for the module is purely to bridge the functionality of Search API as a GraphQL wrapper. These start to get into client consumer considerations which in my opinion belong in a custom module. In our use case we actually have that configuration at the GraphQL module level (not just Search API) as it's useful to define max age headers for other queries as well. Hope that makes sense and thanks again for the submission.

bronius commented 3 years ago

Mm hmm. This makes sense :) It does feel a bit specific and misplaced. Actually, I had wondered if there were an elegant way to specify max-age per index?

Well, I'll take it back to the shop and try another approach like what you are already doing. Thanks! :)

bronius commented 3 years ago

I closed this bc of the discussion above (thank you for indulging and taking the time to explain!). But, PS, I went ahead and updated my PR (https://github.com/bronius/graphql-search-api/commit/e54932ff380922f29a82a95c3b2e893b9bd24b7a) with the capability to set unique Cache max-age per Search API index. Now it is more directly, specifically GraphQL Search API integration focused.

Cheers :)