Closed aminedhobb closed 6 days ago
@Michaelvilleneuve j'ai répondu à tes commentaires, si certaines choses ne sont pas claires on peut se prendre un moment pour pair review si tu veux !
@Michaelvilleneuve @Holist finalement j'ai fait en sorte de fail si on arrive pas à acquérir le lock du premier coup: https://github.com/gip-inclusion/rdv-insertion/pull/2340/commits/b6427dda0f3e6a7b08d868d0f68937f5bd717634
C'est plus logique et plus performant, ça va juste retenter le job plus tard. Je fais en sorte de ne pas logger ces exceptions sur Sentry du coup.
Lié à #2134 closes #1006 au passage.
Rappel du problème
Lorsque l'on traite les webhooks venant de rdvsp, nous devons nous assurer que nous traitons ces webhooks dans l'ordre dans lequel les évènements se sont produits. Pour les
users
,rdvs
,agents
,motifs
oulieux
nous avons une colonne en DBlast_webhook_update_received_at
dans laquelle on stocke le timestamp du webhook ayant emmené l'update. Cela permet d'être sûr que l'on update la ressource que s'il s'agit de la dernière modification reçue par webhook.Nous n'avons pas ce mécanisme pour les ressources de jointure que sont les
agent_roles
,user_profiles
etreferent_assignations
car ces ressources ne sont jamais mises à jour, elles sont créées et détruites et ces deux évènements peuvent s'enchainer rapidement. Cela conduit à un problème: si par exemple un référent est assigné à un usager et est désassigné rapidement, rien ne nous garantit que côté rdv-i on ne traite pas le webhook de destruction de la ressource avant celui de la création => côté rdv-i le référent sera assigné alors qu'il ne le sera pas sur rdvsp ce qui conduit à une désynchronisationPour résoudre le problème
Implémentation
Mécanisme de locking de nos jobs
Il faut donc un mécanisme de locking de nos jobs où 2 jobs de la même classe ne pourraient pas s'exécuter en même temps en fonction des arguments qui lui sont passées (par exemple le couple id de l'usager et id de l'organisation pour le
ProcessUserProfileJob
).Sidekiq
n'offre pas de mécanisme de locking des jobs (dans sa version gratuite en tous cas). Plusieurs solutions s'offraient donc à nous pour l'implémentation:Piste 1: Migrer de
sidekiq
verssolid_queue
Solid queue
est la nouvelle librairie de queuing officielle de Rails qui enqueue les jobs en base de données (contrairement à sidekiq qui utilise redis).Ça fait donc 2 bonnes raisons de migrer vers cette librairie.
Cependant en jouant un peu avec et en essayant d'y migrer notre projet, j'y ai vu quelques inconvénients:
Ces inconvénients m'ont fait penser (peut-être à tort) qu'il était encore un peu tôt pour migrer, surtout que jusqu'à présent Sidekiq nous a toujours apporté satisfaction.
Dans le même ordre d'idée il y a la solution de passer à la gem
GoodJob
(utilisée par rdvsp), qui offre les mêmes avantages quesolid-queue
mais qui elle est mature. J'ai pensé que passer sur la libraire officielle de Rails était plus tentant.Piste 2: Utiliser une librairie permettant de gérer la concurrence avec Sidekiq
La gem
sidekiq-unique-jobs
permet d'ajouter des contraintes d'unicités sur les jobs enqueued par sidekiq. Elle permet notamment en tweakant un peu de reschedule des jobs dont certains arguments sont similaires.Cependant après l'avoir testé cette solution n'a pas été retenue car:
Piste 3: Implémenter un mécanisme simple chez nous (solution retenue)
On implémente un mécanisme simple de locking via la fonctionnalité
with_advisory_lock
de postgres (appelé via la gemwith_advisory_lock
) qui permet de prévenir l'exécution concurrente de code partageant la même clé.Ce mécanisme est implémenté dans le concern
LockedJobs
qui implémente une méthodeperform_with_lock
qui est appelée via le hookaround_perform
: On wrappe la méthodeperform
du job dans un lock où la clé est retournée via une méthode de classelock_key
qui doit être implémentée au niveau de la classe. Cette méthode prend en entrée les mêmes arguments que la méthodeperform
.Ainsi il suffit d'inclure ce concern et d'implémenter la méthode
lock_key
pour être sûr que deux jobs partageant certains arguments ne soient pas exécutés en parallèle. Plus précisément avec cette méthode les jobs peuvent être exécutés en parallèle mais l'intérieur de la méthodeperform
lui ne peut pas. Si le lock ne peut être acquis le job fail et sera retenté plus tard.Mécanisme d'ordering
Ce mécanisme est implémenté dans le concern
LockedAndOrderedJobs
. En incluant ce concern on lock non seulement l'exécution du jobs (via le mécanisme décrit plus haut), mais en plus on inscrit le timestamp du job dans redis. La clé dans laquelle on inscrit ce timestamp est la même que la clé utilisée pour le locking.Ainsi, avant d'entrer dans la méthode
perform
on check si un timestamp n'existe pas pour cette même ressource. S'il existe et qu'il est postérieur au timestamp du job en question, on skip l'exécution du job.Pour utiliser ce concern il faut juste que le job implémente la méthode de classe
job_timestamp
(en plus de la méthodelock_key
citée plus haut).Dans le futur
Notre mécanisme est donc assez simple et semble répondre aux problèmes rencontrés. Cependant il pourrait quand même être judicieux de passer à
solid_queue
à l'avenir car cela représente pas mal d'avantages (librairie officielle, moins de dépendance vis à vis de redis, plus alignée avec ActiveJob). La migration versActiveJob
précédant cette PR (#2323) faciliterait ce processus.Remarques
users
,rdvs
,agents
,motifs
oulieux
. Le check que l'on fait en db vialast_webhook_update_received_at
devrait suffire puisque le cas de création/destruction rapproché n'arrive pas à priori pour ces ressources (on ne reçoit pas de webhook de destruction pour la plupart d'entre elles)