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

Cache préemptif pour l’endpoint /api/datasets #4427

Merged
merged 7 commits into from
Jan 28, 2025
Merged

Conversation

vdegove
Copy link
Contributor

@vdegove vdegove commented Jan 27, 2025

Cette PR rajoute un mécanisme pour calculer et remplir automatiquement le contenu du cache pour la réponse de l’endpoint principal /api/datasets.

Ce calcul du cache se fait au lancement de l’application puis toutes les 5 minutes, le cache étant configuré pour expirer au bout de 10 minutes. En théorie donc, aucun utilisateur ne devrait obtenir de résultat non caché sur cet endpoint. J’ai toutefois gardé le fonctionnement précédent par sécurité : si jamais le cache venait à ne pas être rempli, l’appel GET sur /api/datasets devrait fonctionner (et re-remplir le cache avec un délai de 10 minutes).

Chez moi ça marche, (mais ce n’est pas testé par des tests en dur), extrait de logs :

[info] [api-cache-genserver] Finished populating cache for /api/datasets
[info] GET /api/datasets/
[debug] Processing with TransportWeb.API.DatasetController.datasets/2
  Parameters: %{}
  Pipelines: [:accept_json, :api, :public_cache]
[info] Value for key api-datasets-index served from cache
[info] Sent 304 in 64ms

Je n’ai pas augmenté le timeout Ecto volontairement : celui-ci est par défaut à 15 secondes et l’ensemble de la fonction générant la payload JSON est calculée à environ 3,5s sur ma machine en local. Je préfère donc pour l’instant observer. J’ai mis des logs en début et fin de fonction principale pour avoir une idée du temps réel en production.

@vdegove vdegove requested a review from a team as a code owner January 27, 2025 14:40
@vdegove vdegove changed the title [WIP] Plus de cache (en attendant plus de cash) Plus de cache (en attendant plus de cash) Jan 27, 2025
@vdegove vdegove changed the title Plus de cache (en attendant plus de cash) Cache autogénéré pour l’endpoint /api/datasets Jan 27, 2025
Copy link
Contributor

@thbar thbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai fait une première passe avec des commentaires.

@vdegove si tu souhaites modifier j'attendrai que tu l'aies fait, avant de tester en local (pour économiser le budget).

Je propose aussi de renommer la PR: ce qu'on fait est parfois appelé "cache préemptif" (ex: https://www.drupal.org/forum/general/general-discussion/2009-08-09/what-is-preemptive-caching), ce qui est un peu plus précis.

@vdegove vdegove changed the title Cache autogénéré pour l’endpoint /api/datasets Cache préemptif pour l’endpoint /api/datasets Jan 28, 2025
@vdegove vdegove requested a review from thbar January 28, 2025 12:20
@callback fetch(cache_key :: binary(), fun(), integer()) :: any

defp impl, do: Application.get_env(:transport, :cache_impl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note de relecture : ce n'est pas effacé, mais juste déplacé plus bas du fait de la visibilité privée.

Copy link
Contributor

@thbar thbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai fait deux propositions, une pour éviter un double calcul "aux frontières", l'autre est une note pour le futur.

J'ai aussi vérifié que le cache préemptif ne tournait pas pendant les tests (et donc ne pollue pas apps/transport/test/transport_web/controllers/api/datasets_controller_test.exs).

J'approuve mais @vdegove je te laisse finaliser sur les deux suggestions avant, et on peut déployer pour tenter ensuite ?

vdegove and others added 2 commits January 28, 2025 14:08
Co-authored-by: Thibaut Barrère <[email protected]>
Co-authored-by: Thibaut Barrère <[email protected]>
@vdegove vdegove enabled auto-merge January 28, 2025 13:09
@vdegove vdegove added this pull request to the merge queue Jan 28, 2025
Merged via the queue into master with commit 51e89df Jan 28, 2025
4 checks passed
@vdegove vdegove deleted the 4420-cache-apis branch January 28, 2025 13:17
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