entrouvert/wcs
entrouvert
/
wcs
14
1
Fork 0
Code Issues Pull Requests 34 Releases Activity

further optimize global timeouts (#85692) #1026

Merged
pducroquet merged 2 commits from wip/85692-optimize-more-global-timeout into main 2024-01-26 15:50:13 +01:00
Conversation 2 Commits 2 Files Changed 2 +19 -8
pducroquet commented 2024-01-15 16:46:59 +01:00
Owner
Copy Link

L'optimisation introduite pour les global timeouts "finalized" est aussi utilisable pour 1st-arrival et latest-arrival.
Dans le cas de latest-arrival, la requête ne correspond pas au strict minimum des données à analyser, mais le code de validation en python est toujours présent et s'occupera donc de retirer les cas à ne pas traiter. On aura dans tous les cas une réduction drastique du volume de données traité en python.
Ces cas ont été identifiés sur la recette, alors que sur la prod les pires cas étaient avec "finalized".

L'optimisation introduite pour les global timeouts "finalized" est aussi utilisable pour 1st-arrival et latest-arrival. Dans le cas de latest-arrival, la requête ne correspond pas au strict minimum des données à analyser, mais le code de validation en python est toujours présent et s'occupera donc de retirer les cas à ne pas traiter. On aura dans tous les cas une réduction drastique du volume de données traité en python. Ces cas ont été identifiés sur la recette, alors que sur la prod les pires cas étaient avec "finalized".
fpeters requested changes 2024-01-15 17:23:58 +01:00
fpeters left a comment
Owner
Copy Link

Il faut attendre #1027 pour ne pas faire les mêmes erreurs ici.

Il faut attendre https://git.entrouvert.org/entrouvert/wcs/pulls/1027 pour ne pas faire les mêmes erreurs ici.
pducroquet force-pushed wip/85692-optimize-more-global-timeout from 468061bb76 to df2c8ab1d0 2024-01-15 17:52:50 +01:00 Compare
pducroquet force-pushed wip/85692-optimize-more-global-timeout from b14723b80b to d7d46f6515 2024-01-15 19:44:32 +01:00 Compare
pducroquet force-pushed wip/85692-optimize-more-global-timeout from d7d46f6515 to d0232b606f 2024-01-15 20:01:03 +01:00 Compare
pducroquet force-pushed wip/85692-optimize-more-global-timeout from d0232b606f to 5c807e3b5e 2024-01-15 20:07:12 +01:00 Compare
pducroquet force-pushed wip/85692-optimize-more-global-timeout from a9ce05e51a to aca6009812 2024-01-16 12:39:34 +01:00 Compare
pducroquet force-pushed wip/85692-optimize-more-global-timeout from aca6009812 to cf541e4435 2024-01-16 13:01:50 +01:00 Compare
pducroquet force-pushed wip/85692-optimize-more-global-timeout from cf541e4435 to 8429834b8a 2024-01-16 13:12:15 +01:00 Compare
pducroquet force-pushed wip/85692-optimize-more-global-timeout from ef320b4f52 to 8429834b8a 2024-01-16 13:39:58 +01:00 Compare
pducroquet reviewed 2024-01-16 14:01:43 +01:00
tests/workflow/test_all.py Outdated
@ -4130,6 +4130,7 @@ def test_global_timeouts_latest_arrival(pub):
assert formdef.data_class().get(formdata1.id).get_criticality_level_object().name == 'green'
formdata1.evolution[-1].time = time.localtime(time.time() - 5 * 86400)
formdata1.evolution[-1].status = 'wf-new'
pducroquet commented 2024-01-16 14:01:43 +01:00
Author
Owner
Copy Link

Ne trouvant pas de logique derrière le comportement qui était utilisé pour ce test, je suis convaincu qu'il s'agit d'un effet de bord qui ne correspond pas à une attente ou un besoin fonctionnel.
Précédemment, le code insérait les entrées suivantes dans la table evolutions:

id | status | time
1 | wf-just_submitted | 2024-01-16 10:20:30
2 | new | 2024-01-16 10:20:30
3 | <NULL> | 2024-01-16 10:20:30

Puis mettait à jour la date de la dernière evolution :

id | status | time
1 | wf-just_submitted | 2024-01-16 10:20:30
2 | new | 2024-01-16 10:20:30
3 | <NULL> | 2024-01-11 10:20:30

Et considérait donc que le workflow était dans l'état new depuis 5 jours suite à un tri par défaut sur la colonne id.
Je ne vois pas pourquoi ce serait le comportement voulu d'un point de vue fonctionnel, d'où cette suggestion de correction.
Si besoin était, je pourrais écrire une requête SQL qui implémenterait aussi ce comportement, mais avant d'écrire quelque chose de bien moins efficace je préfère avoir confirmation de ce fonctionnement, qui n'est présent que dans ce test et nulle part ailleurs.

Ne trouvant pas de logique derrière le comportement qui était utilisé pour ce test, je suis convaincu qu'il s'agit d'un effet de bord qui ne correspond pas à une attente ou un besoin fonctionnel. Précédemment, le code insérait les entrées suivantes dans la table evolutions: ``` id | status | time 1 | wf-just_submitted | 2024-01-16 10:20:30 2 | new | 2024-01-16 10:20:30 3 | <NULL> | 2024-01-16 10:20:30 ``` Puis mettait à jour la date de la dernière evolution : ``` id | status | time 1 | wf-just_submitted | 2024-01-16 10:20:30 2 | new | 2024-01-16 10:20:30 3 | <NULL> | 2024-01-11 10:20:30 ``` Et considérait donc que le workflow était dans l'état new depuis 5 jours suite à un tri par défaut sur la colonne id. Je ne vois pas pourquoi ce serait le comportement voulu d'un point de vue fonctionnel, d'où cette suggestion de correction. Si besoin était, je pourrais écrire une requête SQL qui implémenterait aussi ce comportement, mais avant d'écrire quelque chose de bien moins efficace je préfère avoir confirmation de ce fonctionnement, qui n'est présent que dans ce test et nulle part ailleurs.
fpeters commented 2024-01-16 15:00:14 +01:00
Owner
Copy Link

Oui il y a des raccourcis dans le test mais ça me semble souhaité, ça correspond à plus haut,

    # enter in status 8 days ago
    formdata1.evolution[-1].time = time.localtime(time.time() - 8 * 86400)
    # but get a new comment 1 day ago
    formdata1.evolution.append(Evolution(formdata1))
    formdata1.evolution[-1].time = time.localtime(time.time() - 1 * 86400)

L'ajout d'un commentaire, sans changement de statut, crée une nouvelle ligne, un nouveau moment et c'est celui-ci qui doit être considéré pour le calcul de "latest arrival".

Le test vérifie que c'est bien lui qui est pris en compte et pas la première ligne d'évolution.

Plus loin,

    formdata1.evolution[-1].time = time.localtime(time.time() - 5 * 86400)
    formdata1.store()
    pub.apply_global_action_timeouts()

modifie le timestamp de l'evolution pour vérifier qu'alors le trigger est déclenché, vérification à nouveau que c'esrt bien cette dernière ligne d'évolution qui est prise en compte.

En ajoutant un statut explicite sur la ligne d'évolution ça fait que le test "que se passe-t-il sur un simple ajout de commentaire qui a ajouté une ligne d'evolution mais pas changé le statut" ne teste plus ça.

Oui il y a des raccourcis dans le test mais ça me semble souhaité, ça correspond à plus haut, ``` # enter in status 8 days ago formdata1.evolution[-1].time = time.localtime(time.time() - 8 * 86400) # but get a new comment 1 day ago formdata1.evolution.append(Evolution(formdata1)) formdata1.evolution[-1].time = time.localtime(time.time() - 1 * 86400) ``` L'ajout d'un commentaire, sans changement de statut, crée une nouvelle ligne, un nouveau moment et c'est celui-ci qui doit être considéré pour le calcul de "latest arrival". Le test vérifie que c'est bien lui qui est pris en compte et pas la première ligne d'évolution. Plus loin, ``` formdata1.evolution[-1].time = time.localtime(time.time() - 5 * 86400) formdata1.store() pub.apply_global_action_timeouts() ``` modifie le timestamp de l'evolution pour vérifier qu'alors le trigger est déclenché, vérification à nouveau que c'esrt bien cette dernière ligne d'évolution qui est prise en compte. En ajoutant un statut explicite sur la ligne d'évolution ça fait que le test "que se passe-t-il sur un simple ajout de commentaire qui a ajouté une ligne d'evolution mais pas changé le statut" ne teste plus ça.
pducroquet commented 2024-01-16 17:21:31 +01:00
Author
Owner
Copy Link

Trouvé, en fait ce test a été cassé lors du passage de #65744. Ce test ne respecte pas la règle "Et noter que ce sera toujours le dernier evolution qui sera modifié, jamais les précédents, et peut-être que juste se baser là-dessus serait totalement satisfaisant", et donc un store était manquant.

Trouvé, en fait ce test a été cassé lors du passage de #65744. Ce test ne respecte pas la règle "Et noter que ce sera toujours le dernier evolution qui sera modifié, jamais les précédents, et peut-être que juste se baser là-dessus serait totalement satisfaisant", et donc un store était manquant.
pducroquet marked this conversation as resolved
pducroquet requested review from fpeters 2024-01-16 14:02:40 +01:00
pducroquet changed title from WIP: further optimize global timeouts (#85692) to further optimize global timeouts (#85692) 2024-01-16 14:02:55 +01:00
pducroquet force-pushed wip/85692-optimize-more-global-timeout from 8429834b8a to b02d27d39a 2024-01-16 17:17:52 +01:00 Compare
pducroquet force-pushed wip/85692-optimize-more-global-timeout from b02d27d39a to 55226e336b 2024-01-17 09:29:47 +01:00 Compare
pducroquet force-pushed wip/85692-optimize-more-global-timeout from 55226e336b to ab8471cb50 2024-01-18 08:59:03 +01:00 Compare
pducroquet force-pushed wip/85692-optimize-more-global-timeout from ab8471cb50 to 4582188d55 2024-01-22 17:49:15 +01:00 Compare
fpeters force-pushed wip/85692-optimize-more-global-timeout from 4582188d55 to 806df91081 2024-01-26 14:08:02 +01:00 Compare
fpeters commented 2024-01-26 14:16:44 +01:00
Owner
Copy Link

J'ai ajouté un commit pour réduire un peu la duplication, s'il est ok pour toi je validerai la PR.

J'ai ajouté un commit pour réduire un peu la duplication, s'il est ok pour toi je validerai la PR.
pducroquet commented 2024-01-26 15:25:36 +01:00
Author
Owner
Copy Link

J'ai ajouté un commit pour réduire un peu la duplication, s'il est ok pour toi je validerai la PR.

C'est ok pour moi, merci

> J'ai ajouté un commit pour réduire un peu la duplication, s'il est ok pour toi je validerai la PR. C'est ok pour moi, merci
fpeters approved these changes 2024-01-26 15:35:24 +01:00
pducroquet merged commit e4209bca85 into main 2024-01-26 15:50:13 +01:00
pducroquet deleted branch wip/85692-optimize-more-global-timeout 2024-01-26 15:50:13 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
fpeters
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/wcs#1026
No description provided.
Delete Branch "wip/85692-optimize-more-global-timeout"

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?