[TECH] Amélioration de PGBoss (reprise de la PR 11308). #11431
Conversation
|
Une fois les applications déployées, elles seront accessibles via les liens suivants :
Les variables d'environnement seront accessibles via les liens suivants : |
HEYGUL
added
👀 Tech Review Needed
👀 Func Review Needed
| const rowCount = results.reduce((total, batchResult) => total + (batchResult.rowCount || 0), 0); | ||
|
|
||
| return { rowCount }; | ||
| await pgBoss.insert(jobs); |
There was a problem hiding this comment.
J'avais fais la remontee sur la PR de rollback, on perd ici le transactionnel en passant a insert.
Dans le cas detecte sur la PR qui a ete rollback, nous avons constate que en cas d'echec de publication des jobs (batchInsert avant), la transaction (qui existe bien) etait donc pas rollback avec insert ca met des elements de Certif en un etat bancal.
Il semble que il existe justement la possibilite de le mettre "transaction aware", mais avec send et pas insert (via Connection options).
Je souhaiterais que l'on garde le transactionnel pour rester iso au fonctionnement precedent.
There was a problem hiding this comment.
Autre element, avez-vous pu verifier le point sur la PR de rollback concernant le fait que nous ne voyons pas d'erreurs ou log en cas d'echec des insert (erreur silencieuse).
There was a problem hiding this comment.
le soucis dans le cas présent est que insert ne lève pas d'erreurs quand la base n'est pas ouverte au préalable : https://github.com/timgit/pg-boss/blob/067fff74ed9973f6fa71def1bb43ef84b2d21d56/src/db.js#L27
on peut effectivement remettre la transaction autour de l'insert, au cas où une erreur soit levée bien que la base soit ouverte
There was a problem hiding this comment.
@Steph0 je viens de pousser un commit remettant la transaction, peux-tu me dire si c'est ok ?
|
@HEYGUL sur scaling j'ai mis START_IN_WEB_PROCESS = false + j'ai scale les workers a 1, sinon on peut pas verifier que ca fonctionne |
HEYGUL
force-pushed
the
revert-11381-tech-revert-improve-pg-boss-commits
branch
2 times, most recently
from
d76bc97 to
07c5baa
Compare
when worker and server are on two distinct containers, pgBoss shall be started on both
HEYGUL
force-pushed
the
revert-11381-tech-revert-improve-pg-boss-commits
branch
from
07c5baa to
b3ce581
Compare
| @@ -62,7 +58,10 @@ export class JobRepository { | |||
| } | |||
|
|
|||
| async #send(jobs) { | |||
| await pgBoss.insert(jobs); | |||
| const knexConn = DomainTransaction.getConnection(); | |||
| await knexConn.transaction(async () => { | |||
There was a problem hiding this comment.
suggestion : ajouter un test pour la transaction
🥞 Problème
On utilise directement les tables et schémas de base de données de pgBoss.
Cela a pour inconvénient de coupler notre code aux évolutions internes de pgBoss.
🥓 Proposition
pgBoss offre des fonctions natives d'abstraction pour toutes les tâches nécessaires : ajout de job, récupérer les jobs programmés, etc.
On peut donc modifier notre code pour s'affranchir des appels aux tables
pgboss.*.On peut aussi modifier les helpers de test.
🧃 Remarques
Suite à un soucis lorsque le worker n'est pas dans le conteneur web, la PR initiale avait été "annulée".
Le pb venait du fait que pgBoss n'était pas démarré dans le conteneur web (car cela n'était fait que par l'instanciation du worker).
➡️ Voir le dernier commit de la branche.
😋 Pour tester
Tests à définir par les équipes.
Tests accès:
Test certification:
Pix certif : créer une certif
Pix App : passer une certification jusqu'au bout (écran de fin de test)
Pix Admin : voir que la certification n'est pas en statut démarré, ni dans la section a traiter, et que son score est OK
Niveau tech : tout est OK si on voit un événement
CertificationCompletedJobpour le certification-course dans la table pgboss.job