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

[BUGFIX] Rafraichir le cache LCMS en asynchrone (pgboss) #9422

Merged

Conversation

bpetetot
Copy link
Contributor

@bpetetot bpetetot commented Jul 2, 2024

🦄 Problème

Lors d'un rafraîchissement du cache du référentiel de contenu (LCMS) - via Pix Admin par exemple - le conteneur traitant la requête consomme l'intégralité de sa RAM et finit par crasher.
Cela est du au fait que la taille de la release de référentiel est devenue trop importante pour en mettre 2 dans la RAM du même conteneur.

🤖 Proposition

La mise à jour du cache n'est pas une opération qui nécessite d'être synchrone.
On met donc en place un job asynchrone (en utilisant pgboss) pour la mise à jour du cache.
Le cache-controller appelle désormais un usecase dont la responsabilité est d'ajouter un job de mise à jour du cache à la file de job asynchrone.

🌈 Remarques

💯 Pour tester

Se connecter à Pix Admin.
Lancer une mise à jour du cache.
Voir que cette mise à jour s'effectue sans soucis.

@bpetetot bpetetot self-assigned this Jul 2, 2024
@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@bpetetot bpetetot force-pushed the tech-perform-lcms-cache-refresh-into-worker branch from 3afa51b to 1caf68b Compare July 8, 2024 20:09
@bpetetot bpetetot marked this pull request as ready for review July 8, 2024 20:10
@HEYGUL HEYGUL force-pushed the tech-perform-lcms-cache-refresh-into-worker branch from 1caf68b to bcdd96d Compare July 8, 2024 20:22
Copy link
Member

@nlepage nlepage left a comment

Choose a reason for hiding this comment

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

Ça marche à merveille 👌

}

get name() {
return LcmsRefreshCacheJob.name;
Copy link
Member

Choose a reason for hiding this comment

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

question: Il n'y a pas de propriété statique name sur LcmsRefreshCacheJob, ça fonctionne quand même ?

Copy link
Member

Choose a reason for hiding this comment

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

C'est JobPgBoss qui stocke la variable, dans le super de LcmsRefreshCacheJob

Copy link
Contributor

Choose a reason for hiding this comment

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

@nlepage c'est pas une propriété de JobPgBoss ?

Copy link
Contributor Author

@bpetetot bpetetot Jul 9, 2024

Choose a reason for hiding this comment

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

En Javascript, la propriété statique name d'une classe ou fonction retourne son nom:

class LcmsRefreshCacheJob {}
console.log(LcmsRefreshCacheJob.name) // returns 'LcmsRefreshCacheJob'

Ce qui n'est pas top c'est qu'on initialise également une propriété name du job dans le constructeur (qui peut potentiellement différer). Et il est probable que si elle est différente, le job ne se lance pas. C'est le cas pour tous les jobs. 🤔

class LcmsRefreshCacheJob extends JobPgBoss {
  constructor(queryBuilder) {
    super({ name: 'LcmsRefreshCacheJob', retryLimit: 0 }, queryBuilder);
  }
}

Ça serait potentiellement un point à améliorer pour l'équipe "Évènementiel" des tech days.

Copy link
Member

@VincentHardouin VincentHardouin left a comment

Choose a reason for hiding this comment

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

LGTM, je me demande juste si c'est pas un BUGFIX : réparer le rafraichissement du cache depuis l'interface, WDYT ?

Comment on lines +6 to +7
const refreshCacheEntries = async function (_, h) {
await usecases.refreshLearningContentCache();
Copy link
Contributor

Choose a reason for hiding this comment

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

pourquoi on fait pas de la "vraie" inversion de dépendance ici ? (en déclarant les usecases en argument du controller)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dans tous les controllers de Pix API, on importe les usecases en dépendance node il me semble. Aujourd'hui il n'y a que les serializers injecté en argument des controlleurs.

Copy link
Contributor

@dlahaye dlahaye Jul 9, 2024

Choose a reason for hiding this comment

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

il y a des exemples dans le code où les usecases sont injectés

https://github.com/1024pix/pix/blob/dev/api/src/devcomp/application/modules/controller.js
https://github.com/1024pix/pix/blob/dev/api/src/devcomp/application/passages/controller.js

mais oui après avoir checké j'ai l'impression qu'on fait ça seulement chez devcomp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ^^ Injecter les usecases comme le reste me semble bien vis-à-vis de notre archi, peut-être qu'on devrait avoir une guideline globale là-dessus.

Copy link
Contributor

@dlahaye dlahaye Jul 9, 2024

Choose a reason for hiding this comment

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

En fait l'ADR 46 dit bien que c'est une volonté de ne pas injecter les usecases dans les controller mais ce n'est pas dit pourquoi (@frinyvonnick ?)

}

get name() {
return LcmsRefreshCacheJob.name;
Copy link
Contributor

Choose a reason for hiding this comment

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

@nlepage c'est pas une propriété de JobPgBoss ?

@MathieuGilet
Copy link
Contributor

Bérengère vient de penser à un truc concernant cette PR: est ce qu'on ne devrait pas modifier aussi le script api/scripts/refresh-cache.js qui est appelé lorsque l'on fait un node run cache:refresh ?

@bpetetot
Copy link
Contributor Author

bpetetot commented Jul 9, 2024

Bérengère vient de penser à un truc concernant cette PR: est ce qu'on ne devrait pas modifier aussi le script api/scripts/refresh-cache.js qui est appelé lorsque l'on fait un node run cache:refresh ?

Le script est déjà exécuté dans un conteneur dédié (qu'on peut sizer comme on le souhaite). Je ne pense pas que ça soit nécessaire de le lancer dans un job. Il faut voir le script comme un autre moyen de faire le refresh. Si on le met dans un job comme via l'interface d'admin, dans ce cas il ne sert plus à rien, autant toujours passer via l'interface.

@bpetetot bpetetot changed the title [TECH] Rafraichir le cache LCMS en asynchrone (pgboss) [BUGFIX] Rafraichir le cache LCMS en asynchrone (pgboss) Jul 9, 2024
@pix-service-auto-merge pix-service-auto-merge force-pushed the tech-perform-lcms-cache-refresh-into-worker branch from bcdd96d to fcccb95 Compare July 9, 2024 16:28
@pix-service-auto-merge pix-service-auto-merge merged commit d2ad433 into dev Jul 9, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-team Toutes les équipes de dev Func Review OK PO validated functionally the PR 🚀 Ready to Merge Tech Review OK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants