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

Passer de delayed_job à good_job #3338

Merged
merged 14 commits into from
Feb 23, 2023
Merged

Passer de delayed_job à good_job #3338

merged 14 commits into from
Feb 23, 2023

Conversation

francois-ferrandis
Copy link
Contributor

@francois-ferrandis francois-ferrandis commented Feb 9, 2023

Fixes #2231

Pourquoi ?

Nos problématiques actuelles avec DelayedJob :

  • pas de visibilité sur les jobs qui se sont exécutés avec succès (ils sont supprimés immédiatement)
  • fonctionnalités de retry limitées
  • API différente entre DelayedJob et ActiveJob, ce qui rend difficile de savoir quelles fonctionnalités sont dispo

GoodJob répond à ces problématiques tout en apportant d'autres "nice to have" pour nous :

  • affichage de la durée d'exécution des jobs
  • chaque job a un ID, et donc une page associée vers laquelle on peut pointer
  • affichage d'un graphique des dernières 24h pour voir les volumes de jobs selon la queue
  • fonctionnalité de cron intégrée dans la gem
  • exécutions multi-thread (ça dépile plus vite)

De manière générale, GoodJob est plus moderne, plus expert et plus agréable à utiliser que DelayedJob, ce qui devrait apporter un plaisir non négligeable dans notre travail. Je recommande la lecture du README de GoodJob qui aborde de nombreuses problématiques propres à la gestion de jobs, et démontrent ainsi l'expertise du concepteur de la gem.

Comment ?

Changement dans le code

La migration d'un système à l'autre se fait assez simplement :

  • mise à jour du Procfile
  • déplacement de la config des tâches cron vers la fichier de config good_job.rb
  • mise à jour des routes

Les retries

Le changement le plus subtil concerne les retries. En effet, avec ActiveJob (et donc par défaut dans GoodJob), un job qui échoue n'est pas ré-essayé, il est simplement marqué "discarded" (et donc visible dans la section "Dsicarded" du dashboard). Afin qu'un job soit ré-essayé, il faut utiliser la méthode retry_on (une méthode standard de ActiveJob, cf. doc de GoodJob sur ce sujet).

Jusqu'à maintenant, chaque échec d’exécution était envoyé à Sentry à chaque retry. J'ai donc fait en sorte de garder ce comportement pour le moment, et nous pourrons ensuite modifier chaque job au cas par cas pour décidé si chaque retry a besoin d'être loggué dans Sentry ou pas, le nombre de retries, leur fréquence, tout ça selon le type d'exception levée ! 😋

J'ai défini le nombre de retry avant abandon à 20, ce qui signifie qu'on abandonne environ 24h après la première exécution. Ça me paraît judicieux, car actuellement on est à 25, ce qui fait qu'on a par exemple des jobs de notifs ("votre RDV est après demain") qui ré-essayent pendant 2 semaines. 😬 Je travaille sur une seconde PR qui propose des ajustement spécifiques à chaque job et chaque erreur.

Migration

Je pense que le plus simple est de ne pas migrer les jobs existants dans la table delayed_jobs. En effet, en temps normal, tous les jobs stockés dans cette table sont en retry infini car ils déclenchent une erreur irrémédiable (webhook qui prend une 500 de chez Zimbra, SMS qui ne peut pas être routé car le numéro a changé de fournisseur). Nous pouvons garder la table delayed_jobs dans la base pour le moment, puis la supprimer quand nous aurons vérifié que les jobs qu'elle contient sont à jeter.

Checklist

Avant la revue :

  • Nettoyer les commits pour faciliter la relecture
  • Supprimer les éventuels logs de test et le code mort

Revue :

  • Relecture du code
  • Test sur la review app / en local

Comment on lines -9 to -10
around { |example| perform_enqueued_jobs { example.run } }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En exécutant toujours les jobs de façon synchrone, ces tests échouaient car ils exécutaient les 20 retries. Je suis donc passé à un mode de test où l'on appelle #deliver_later pour mettre le job à la file, puis perform_enqueued_jobs pour exécuter le job qui est dans la file, et vérifier qu'il a bien contacté Sentry ou pas, et déclenché un retry ou pas. 😉

Copy link
Contributor

@Holist Holist left a comment

Choose a reason for hiding this comment

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

C'est top :)
Merci beaucoup. Ca va être un plaisir à utiliser.
A priori avoir fixé à 10 retry les jobs des webhook ne devrait pas poser de problèmes particuliers pour rdv-i (mais juste pour info @aminedhobb )

Procfile.dev Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Configurer plusieurs jobs en parallèle pour les workers
2 participants