entrouvert
/
hobo
16
0
Fork 0
Code Issues Pull Requests 15 Releases Activity

application: lorsqu'une brique traite une tâche en asynchrone, ne pas noter le job comme terminé (#89124) #122

Open
lguerin wants to merge 3 commits from wip/89124-application-job-waiting into main
pull from: wip/89124-application-job-waiting
merge into: entrouvert:main
Conversation 1 Commits 3 Files Changed 6 +137 -49
lguerin commented 2024-04-12 18:20:36 +02:00
Owner
Copy Link
No description provided.
lguerin added 2 commits 2024-04-12 18:20:36 +02:00
lguerin force-pushed wip/89124-application-job-waiting from 0fb6011489 to 2060b8c68d 2024-04-12 18:20:56 +02:00 Compare
lguerin force-pushed wip/89124-application-job-waiting from 2060b8c68d to 2084b900ee 2024-04-15 15:33:05 +02:00 Compare
lguerin force-pushed wip/89124-application-job-waiting from 2084b900ee to 410f7a0f59 2024-04-15 15:45:42 +02:00 Compare
lguerin force-pushed wip/89124-application-job-waiting from 410f7a0f59 to 8f51bbbae0 2024-04-15 16:03:56 +02:00 Compare
lguerin force-pushed wip/89124-application-job-waiting from 8f51bbbae0 to 54f13d8802 2024-04-15 16:12:12 +02:00 Compare
lguerin force-pushed wip/89124-application-job-waiting from 54f13d8802 to 88ac447df8 2024-04-15 16:16:15 +02:00 Compare
lguerin changed title from WIP: application: lorsqu'une brique traite une tâche en asynchrone, ne pas noter le job comme terminé (#89124) to application: lorsqu'une brique traite une tâche en asynchrone, ne pas noter le job comme terminé (#89124) 2024-04-15 16:31:51 +02:00
pmarillonnet requested review from pmarillonnet 2024-04-16 14:50:35 +02:00
pmarillonnet commented 2024-04-16 14:50:47 +02:00
Owner
Copy Link

(Je commence à relire.)

(Je commence à relire.)
pmarillonnet reviewed 2024-04-16 16:35:08 +02:00
pmarillonnet left a comment
Owner
Copy Link

Une première relecture à chaud, avec quelques petites remarques.

Une première relecture à chaud, avec quelques petites remarques.
hobo/applications/models.py
@ -662,0 +679,4 @@
if service_id not in self.progression_cache:
self.progression_cache[service_id] = {}
cache = self.progression_cache[service_id].get(service) or {}
if (cache.get('data') or {}).get('status') == 'completed':
pmarillonnet commented 2024-04-16 16:30:49 +02:00
Owner
Copy Link

Je n’ai pas compris en quoi le critère d’écriture des données du cache dans le contexte est seulement “le statut est marqué comme complété”, par exemple pourquoi ne pas y inclure aussi les modules dont le statut est marqué comme en échec ?

Je n’ai pas compris en quoi le critère d’écriture des données du cache dans le contexte est seulement “le statut est marqué comme complété”, par exemple pourquoi ne pas y inclure aussi les modules dont le statut est marqué comme en échec ?
lguerin commented 2024-04-17 16:49:00 +02:00
Author
Owner
Copy Link

juste en dessous, si on a un module en failed, on note le job en échec; et donc on arrête d'interroger les modules pour avoir leur complétion

juste en dessous, si on a un module en failed, on note le job en échec; et donc on arrête d'interroger les modules pour avoir leur complétion
hobo/applications/models.py
@ -662,0 +702,4 @@
self.exception = 'Failed to deploy modules %s' % ', '.join(failed)
else:
self.exception = 'Failed to deploy module %s' % failed[0]
self.completion_timestamp = now()
pmarillonnet commented 2024-04-16 15:46:58 +02:00
Owner
Copy Link

Est-ce qu’il n’y a pas un danger à écrire le completion_timestamp ici, juste parce que certains modules sont en échecs, alors que d’autres sont peut-être en cours de progression et que le job n’est pas complété à proprement parler ?
Ne faudrait-il pas l’écrire seulement si tous les modules qui ne sont pas en statut failed sont alors en statut completed ?

Est-ce qu’il n’y a pas un danger à écrire le `completion_timestamp` ici, juste parce que certains modules sont en échecs, alors que d’autres sont peut-être en cours de progression et que le job n’est pas complété à proprement parler ? Ne faudrait-il pas l’écrire seulement si tous les modules qui ne sont pas en statut `failed` sont alors en statut `completed` ?
lguerin commented 2024-04-17 16:52:48 +02:00
Author
Owner
Copy Link

Code déplacé, on settait déjà completion_timestamp avant en cas d'échec.
Et si tu regardes la méthode run, dans le finally on set tout le temps completion_timestamp.
Je trouve ça un peu bancal aussi, mais je préfèrerais qu'on change cette logique dans un aure ticket.

Code déplacé, on settait déjà completion_timestamp avant en cas d'échec. Et si tu regardes la méthode run, dans le finally on set tout le temps completion_timestamp. Je trouve ça un peu bancal aussi, mais je préfèrerais qu'on change cette logique dans un aure ticket.
pylint.rc
@ -55,6 +55,7 @@ disable=
unnecessary-lambda-assignment,
unspecified-encoding,
unsubscriptable-object,
unsupported-assignment-operation,
pmarillonnet commented 2024-04-16 16:34:28 +02:00
Owner
Copy Link

Par curiosité c’est quelle partie de ce commit qui nécessite de désactiver cette erreur pylint ?
Je me demande si on aurait pas intérêt à poser cette désactivation en inline juste pour la ligne concernée. Ça me paraît un truc pertinent à avoir nonobstant le faux-positif dans ce commit.

Par curiosité c’est quelle partie de ce commit qui nécessite de désactiver cette erreur pylint ? Je me demande si on aurait pas intérêt à poser cette désactivation en inline juste pour la ligne concernée. Ça me paraît un truc pertinent à avoir nonobstant le faux-positif dans ce commit.
lguerin commented 2024-04-17 16:54:18 +02:00
Author
Owner
Copy Link

ici: https://jenkins.entrouvert.org/job/gitea/job/hobo/job/wip%252F89124-application-job-waiting/3/pylint/new/source.c371fafc-eab4-49b7-bb50-1e27049a3d07/#680

En fait on a le même disabled dans pylint.rc dans chrono, j'ai fait pareil.

ici: https://jenkins.entrouvert.org/job/gitea/job/hobo/job/wip%252F89124-application-job-waiting/3/pylint/new/source.c371fafc-eab4-49b7-bb50-1e27049a3d07/#680 En fait on a le même disabled dans pylint.rc dans chrono, j'ai fait pareil.
All checks were successful
gitea/hobo/pipeline/head This commit looks good
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b wip/89124-application-job-waiting main
git pull origin wip/89124-application-job-waiting

Step 2:

Merge the changes and update on Gitea.
git checkout main
git merge --no-ff wip/89124-application-job-waiting
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
pmarillonnet
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/hobo#122
No description provided.
Delete Branch "wip/89124-application-job-waiting"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?