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

global actions timeout: use SQL for finalized triggers (#85622) #1022

Merged
pducroquet merged 1 commits from wip/85622-optimize-action-global-timeout into main 2024-01-15 11:41:34 +01:00
Conversation 6 Commits 1 Files Changed 3 +74 -26
pducroquet commented 2024-01-13 12:37:22 +01:00
Owner
Copy Link
No description provided.
fpeters requested changes 2024-01-13 14:03:21 +01:00
fpeters left a comment
Owner
Copy Link

J'ai l'impression qu'il manque du fonctionnement le fait que les déclencheurs se déclenchent une seule fois :

                    if trigger.id in seen_triggers:
                        continue  # already triggered
J'ai l'impression qu'il manque du fonctionnement le fait que les déclencheurs se déclenchent une seule fois : ``` if trigger.id in seen_triggers: continue # already triggered ```
fpeters reviewed 2024-01-13 14:13:59 +01:00
wcs/sql_criterias.py Outdated
@ -435,0 +446,4 @@
statuses = 'c%s' % id(self.value)
formdef_table = self.formdef._table_name
formdef_evolution_table = self.formdef._table_name + '_evolutions'
return f'''EXISTS(
fpeters commented 2024-01-13 14:13:59 +01:00
Owner
Copy Link

Du ticket :

La traduction du critère «première transition dans l'état donné» est fait par un EXISTS sur la table evolutions, l'ordre n'ayant finalement pas d'importance, et cela évite donc les constructions SQL les plus complexes.

Je ne capte pas ce qui est entendu par "première transition dans l'état donné", j'aurais imaginé que c'était le cas trigger.anchor == '1st-arrival' mais comme seule le cas "finalized" est traité, je suis perdu.

Du ticket : > La traduction du critère «première transition dans l'état donné» est fait par un EXISTS sur la table evolutions, l'ordre n'ayant finalement pas d'importance, et cela évite donc les constructions SQL les plus complexes. Je ne capte pas ce qui est entendu par "première transition dans l'état donné", j'aurais imaginé que c'était le cas `trigger.anchor == '1st-arrival'` mais comme seule le cas "finalized" est traité, je suis perdu.
pducroquet commented 2024-01-13 14:53:25 +01:00
Author
Owner
Copy Link

Je me suis embrouillé pardon, pour finalized le code regarde la dernière transition dans l'état donné. Je me rends compte que j'ai perdu une subtilité, mais je m'interroge sur son intérêt pour un état final. Si on a une transition vers un état final, puis vers un autre état, puis à nouveau vers l'état final, c'est la date de la première transition qui sera prise en compte. Est-ce-que j'altère le SQL pour prendre ce critère en compte ?

Je me suis embrouillé pardon, pour finalized le code regarde la dernière transition dans l'état donné. Je me rends compte que j'ai perdu une subtilité, mais je m'interroge sur son intérêt pour un état final. Si on a une transition vers un état final, puis vers un autre état, puis à nouveau vers l'état final, c'est la date de la première transition qui sera prise en compte. Est-ce-que j'altère le SQL pour prendre ce critère en compte ?
pducroquet commented 2024-01-13 16:40:12 +01:00
Author
Owner
Copy Link

Précision : prendre ce critère en compte veut juste dire "réduire le risque de faux positifs" dans la requête SQL. Le code qui choisit et valide la date à prendre est toujours présent, ce qui fait que si le SQL renvoit plus d'objets qu'il n'en faut, ce n'est pas un problème.

Précision : prendre ce critère en compte veut juste dire "réduire le risque de faux positifs" dans la requête SQL. Le code qui choisit et valide la date à prendre est toujours présent, ce qui fait que si le SQL renvoit plus d'objets qu'il n'en faut, ce n'est pas un problème.
fpeters commented 2024-01-13 18:09:10 +01:00
Owner
Copy Link

Est-ce-que j'altère le SQL pour prendre ce critère en compte ?

Oui il me semble important que ça se déclenche sur la même situation qu'avant.

> Est-ce-que j'altère le SQL pour prendre ce critère en compte ? Oui il me semble important que ça se déclenche sur la même situation qu'avant.
pducroquet commented 2024-01-14 11:39:27 +01:00
Author
Owner
Copy Link

Ha mais ça ne change pas du tout le déclenchement. Là on ne fait que filtrer ce sur quoi on va travailler, le code qui vérifie le déclenchement reste en place bien entendu.
Sur toulouse je mesure moins de 3% des formdata qui sont passés deux fois par un état final, donc je considère que c'est suffisamment peu fréquent pour qu'on ne complique pas considérablement le code pour optimiser ça (en tout cas pour le moment).

Ha mais ça ne change pas du tout le déclenchement. Là on ne fait que filtrer ce sur quoi on va travailler, le code qui vérifie le déclenchement reste en place bien entendu. Sur toulouse je mesure moins de 3% des formdata qui sont passés deux fois par un état final, donc je considère que c'est suffisamment peu fréquent pour qu'on ne complique pas considérablement le code pour optimiser ça (en tout cas pour le moment).
pducroquet commented 2024-01-13 14:54:27 +01:00
Author
Owner
Copy Link

J'ai l'impression qu'il manque du fonctionnement le fait que les déclencheurs se déclenchent une seule fois :

                    if trigger.id in seen_triggers:
                        continue  # already triggered

En effet, je l'ai perdu en réécrivant la boucle. J'ai envoyé un commit qui reprend ça.

> J'ai l'impression qu'il manque du fonctionnement le fait que les déclencheurs se déclenchent une seule fois : > > ``` > if trigger.id in seen_triggers: > continue # already triggered > ``` En effet, je l'ai perdu en réécrivant la boucle. J'ai envoyé un commit qui reprend ça.
pducroquet force-pushed wip/85622-optimize-action-global-timeout from 418b751997 to b08f6dd5bb 2024-01-13 15:01:35 +01:00 Compare
pducroquet requested review from fpeters 2024-01-13 17:33:48 +01:00
fpeters requested changes 2024-01-13 18:09:19 +01:00
fpeters left a comment
Owner
Copy Link

J'ai l'impression qu'il manque du fonctionnement le fait que les déclencheurs se déclenchent une seule fois :

                    if trigger.id in seen_triggers:
                        continue  # already triggered

En effet, je l'ai perdu en réécrivant la boucle. J'ai envoyé un commit qui reprend ça.

Il me semble que non, il s'agit de ne pas, pour un formdata, jouer plusieurs fois un déclencheur; dans la boucle d'origine on fait tourner ça par formdata :

            for formdata in data_class.select_iterator(clause=criterias, itersize=200):
(...)
                seen_triggers = []
                for part in formdata.iter_evolution_parts(WorkflowGlobalActionTimeoutTriggerMarker):
                    seen_triggers.append(part.timeout_id)

                for action, trigger in triggers:
                    if trigger.id in seen_triggers:
                        continue  # already triggered

vs dans ton ajout, où ça passe en haut de la boucle,

        seen_triggers = []
        for action, trigger in finalized_triggers:
            if trigger.id in seen_triggers:
                continue
            seen_triggers.append(trigger.id)

et ça n'aura pas d'incidence sur les formdata, et l'exécution multiple pourra y avoir lieu.

A priori cette modification aux tests permet de détecter la situation :

--- a/tests/workflow/test_all.py
+++ b/tests/workflow/test_all.py
@@ -3828,6 +3828,8 @@ def test_global_timeouts(pub, formdef_class):
     workflow.store()
     pub.apply_global_action_timeouts()
     assert formdef.data_class().get(formdata1.id).get_criticality_level_object().name == 'yellow'
+    pub.apply_global_action_timeouts()
+    assert formdef.data_class().get(formdata1.id).get_criticality_level_object().name == 'yellow'
     formdata1.store()

J'ai ajouté un commit avec ce code.

> > J'ai l'impression qu'il manque du fonctionnement le fait que les déclencheurs se déclenchent une seule fois : > > > > ``` > > if trigger.id in seen_triggers: > > continue # already triggered > > ``` > > En effet, je l'ai perdu en réécrivant la boucle. J'ai envoyé un commit qui reprend ça. Il me semble que non, il s'agit de ne pas, pour un formdata, jouer plusieurs fois un déclencheur; dans la boucle d'origine on fait tourner ça par formdata : ``` for formdata in data_class.select_iterator(clause=criterias, itersize=200): (...) seen_triggers = [] for part in formdata.iter_evolution_parts(WorkflowGlobalActionTimeoutTriggerMarker): seen_triggers.append(part.timeout_id) for action, trigger in triggers: if trigger.id in seen_triggers: continue # already triggered ``` vs dans ton ajout, où ça passe en haut de la boucle, ``` seen_triggers = [] for action, trigger in finalized_triggers: if trigger.id in seen_triggers: continue seen_triggers.append(trigger.id) ``` et ça n'aura pas d'incidence sur les formdata, et l'exécution multiple pourra y avoir lieu. A priori cette modification aux tests permet de détecter la situation : ``` --- a/tests/workflow/test_all.py +++ b/tests/workflow/test_all.py @@ -3828,6 +3828,8 @@ def test_global_timeouts(pub, formdef_class): workflow.store() pub.apply_global_action_timeouts() assert formdef.data_class().get(formdata1.id).get_criticality_level_object().name == 'yellow' + pub.apply_global_action_timeouts() + assert formdef.data_class().get(formdata1.id).get_criticality_level_object().name == 'yellow' formdata1.store() ``` J'ai ajouté un commit avec ce code.
pducroquet commented 2024-01-14 11:51:29 +01:00
Author
Owner
Copy Link

J'ai l'impression qu'il manque du fonctionnement le fait que les déclencheurs se déclenchent une seule fois :

                    if trigger.id in seen_triggers:
                        continue  # already triggered

En effet, je l'ai perdu en réécrivant la boucle. J'ai envoyé un commit qui reprend ça.

Il me semble que non, il s'agit de ne pas, pour un formdata, jouer plusieurs fois un déclencheur; dans la boucle d'origine on fait tourner ça par formdata :

            for formdata in data_class.select_iterator(clause=criterias, itersize=200):
(...)
                seen_triggers = []
                for part in formdata.iter_evolution_parts(WorkflowGlobalActionTimeoutTriggerMarker):
                    seen_triggers.append(part.timeout_id)

                for action, trigger in triggers:
                    if trigger.id in seen_triggers:
                        continue  # already triggered

vs dans ton ajout, où ça passe en haut de la boucle,

        seen_triggers = []
        for action, trigger in finalized_triggers:
            if trigger.id in seen_triggers:
                continue
            seen_triggers.append(trigger.id)

et ça n'aura pas d'incidence sur les formdata, et l'exécution multiple pourra y avoir lieu.

A priori cette modification aux tests permet de détecter la situation :

--- a/tests/workflow/test_all.py
+++ b/tests/workflow/test_all.py
@@ -3828,6 +3828,8 @@ def test_global_timeouts(pub, formdef_class):
     workflow.store()
     pub.apply_global_action_timeouts()
     assert formdef.data_class().get(formdata1.id).get_criticality_level_object().name == 'yellow'
+    pub.apply_global_action_timeouts()
+    assert formdef.data_class().get(formdata1.id).get_criticality_level_object().name == 'yellow'
     formdata1.store()

J'ai ajouté un commit avec ce code.

Merci, je comprends mieux l'idée. J'ai corrigé en ce sens normalement.

> > > J'ai l'impression qu'il manque du fonctionnement le fait que les déclencheurs se déclenchent une seule fois : > > > > > > ``` > > > if trigger.id in seen_triggers: > > > continue # already triggered > > > ``` > > > > En effet, je l'ai perdu en réécrivant la boucle. J'ai envoyé un commit qui reprend ça. > > Il me semble que non, il s'agit de ne pas, pour un formdata, jouer plusieurs fois un déclencheur; dans la boucle d'origine on fait tourner ça par formdata : > > ``` > for formdata in data_class.select_iterator(clause=criterias, itersize=200): > (...) > seen_triggers = [] > for part in formdata.iter_evolution_parts(WorkflowGlobalActionTimeoutTriggerMarker): > seen_triggers.append(part.timeout_id) > > for action, trigger in triggers: > if trigger.id in seen_triggers: > continue # already triggered > ``` > > vs dans ton ajout, où ça passe en haut de la boucle, > > ``` > seen_triggers = [] > for action, trigger in finalized_triggers: > if trigger.id in seen_triggers: > continue > seen_triggers.append(trigger.id) > ``` > > et ça n'aura pas d'incidence sur les formdata, et l'exécution multiple pourra y avoir lieu. > > A priori cette modification aux tests permet de détecter la situation : > > ``` > --- a/tests/workflow/test_all.py > +++ b/tests/workflow/test_all.py > @@ -3828,6 +3828,8 @@ def test_global_timeouts(pub, formdef_class): > workflow.store() > pub.apply_global_action_timeouts() > assert formdef.data_class().get(formdata1.id).get_criticality_level_object().name == 'yellow' > + pub.apply_global_action_timeouts() > + assert formdef.data_class().get(formdata1.id).get_criticality_level_object().name == 'yellow' > formdata1.store() > ``` > > J'ai ajouté un commit avec ce code. Merci, je comprends mieux l'idée. J'ai corrigé en ce sens normalement.
pducroquet force-pushed wip/85622-optimize-action-global-timeout from a594b576b1 to bf7071f416 2024-01-14 11:59:15 +01:00 Compare
pducroquet requested review from fpeters 2024-01-14 12:06:58 +01:00
fpeters commented 2024-01-14 13:57:14 +01:00
Owner
Copy Link

Ok ça tourne mais je ne le sens pas encore (désolé pour la subjectivité), et j'aurais dû commencer par là, je manque d'éléments d'analyse, tu pourrais remonter et partager davantage de celle-ci, type pointer une démarche précise, l'action de workflow concernée, etc. ?

Particulièrement, de la description initiale "consommation massive de bande passante" je serais curieux de voir si celle-ci vient du nombre de lignes d'évolution ou de la présence de gros "parts" dedans.

(édit : j'entends que le ticket a été écrit vaguement parce que public, ces précisions peuvent bien sûr aller dans un commentaire privé)

Ok ça tourne *mais* je ne le sens pas encore (désolé pour la subjectivité), et j'aurais dû commencer par là, je manque d'éléments d'analyse, tu pourrais remonter et partager davantage de celle-ci, type pointer une démarche précise, l'action de workflow concernée, etc. ? Particulièrement, de la description initiale "consommation massive de bande passante" je serais curieux de voir si celle-ci vient du nombre de lignes d'évolution ou de la présence de gros "parts" dedans. (édit : j'entends que le ticket a été écrit vaguement parce que public, ces précisions peuvent bien sûr aller dans un commentaire privé)
pducroquet commented 2024-01-14 14:20:26 +01:00
Author
Owner
Copy Link

Ok ça tourne mais je ne le sens pas encore (désolé pour la subjectivité), et j'aurais dû commencer par là, je manque d'éléments d'analyse, tu pourrais remonter et partager davantage de celle-ci, type pointer une démarche précise, l'action de workflow concernée, etc. ?

Particulièrement, de la description initiale "consommation massive de bande passante" je serais curieux de voir si celle-ci vient du nombre de lignes d'évolution ou de la présence de gros "parts" dedans.

(édit : j'entends que le ticket a été écrit vaguement parce que public, ces précisions peuvent bien sûr aller dans un commentaire privé)

Fait sur une note privée.

> Ok ça tourne *mais* je ne le sens pas encore (désolé pour la subjectivité), et j'aurais dû commencer par là, je manque d'éléments d'analyse, tu pourrais remonter et partager davantage de celle-ci, type pointer une démarche précise, l'action de workflow concernée, etc. ? > > Particulièrement, de la description initiale "consommation massive de bande passante" je serais curieux de voir si celle-ci vient du nombre de lignes d'évolution ou de la présence de gros "parts" dedans. > > (édit : j'entends que le ticket a été écrit vaguement parce que public, ces précisions peuvent bien sûr aller dans un commentaire privé) Fait sur une note privée.
fpeters commented 2024-01-14 14:27:48 +01:00
Owner
Copy Link

Fait sur une note privée.

Ok merci.

> Fait sur une note privée. Ok merci.
fpeters requested changes 2024-01-14 14:51:07 +01:00
wcs/sql_criterias.py Outdated
@ -435,0 +435,4 @@
class SpecialTimeoutCriteria(Criteria):
def __init__(self, formdef, statuses, duration, **kwargs):
fpeters commented 2024-01-14 14:29:20 +01:00
Owner
Copy Link

Je verrais vien un nom plus précis, peut-être "GlobalTimeoutOnFinalizedCriteria" ?

Je verrais vien un nom plus précis, peut-être "GlobalTimeoutOnFinalizedCriteria" ?
pducroquet commented 2024-01-14 17:12:38 +01:00
Author
Owner
Copy Link

Comme ce n'est pas spécifique aux GlobalTimeout, j'ai préféré StatusReachedTimeoutCriteria, ça te va ?

Comme ce n'est pas spécifique aux GlobalTimeout, j'ai préféré StatusReachedTimeoutCriteria, ça te va ?
wcs/sql_criterias.py Outdated
@ -435,0 +446,4 @@
statuses = 'c%s' % id(self.value)
formdef_table = self.formdef._table_name
formdef_evolution_table = self.formdef._table_name + '_evolutions'
return f'''EXISTS(
fpeters commented 2024-01-14 14:32:53 +01:00
Owner
Copy Link

J'aimerais bien ici un commentaire explicatif.

# check timeout delay against evolution table, there must be at least one row older than the delay.

?

J'aimerais bien ici un commentaire explicatif. ``` # check timeout delay against evolution table, there must be at least one row older than the delay. ``` ?
pducroquet commented 2024-01-14 16:33:00 +01:00
Author
Owner
Copy Link

fait.

fait.
wcs/sql_criterias.py Outdated
@ -435,0 +450,4 @@
SELECT 1 FROM {formdef_evolution_table}
WHERE {formdef_evolution_table}.formdata_id = {formdef_table}.id
AND {formdef_evolution_table}.status = ANY(%({statuses})s)
AND {formdef_evolution_table}.time < NOW() - {duration} * interval '1 day')'''
fpeters commented 2024-01-14 14:34:09 +01:00
Owner
Copy Link

On est sûr du < plutôt que <= ? Comme ça ne doit pas changer des masses dans le résultat, je jouerais prudent et mettrais <=.

On est sûr du < plutôt que <= ? Comme ça ne doit pas changer des masses dans le résultat, je jouerais prudent et mettrais <=.
pducroquet commented 2024-01-14 16:33:07 +01:00
Author
Owner
Copy Link

fait.

fait.
wcs/workflows.py Outdated
@ -2078,0 +2087,4 @@
# now we need the criteria for our timeout
criterias.append(SpecialTimeoutCriteria(data_class, endpoint_status_ids, trigger.timeout))
# and we can execute on what remains.
for formdata in data_class.select_iterator(clause=criterias, itersize=200):
fpeters commented 2024-01-14 14:50:57 +01:00
Owner
Copy Link

Je serais assez pour ne pas dupliquer cette partie, j'envoie un commit supplémentaire dans la branche pour illustrer ça.

60f2e07189

Je serais assez pour ne pas dupliquer cette partie, j'envoie un commit supplémentaire dans la branche pour illustrer ça. https://git.entrouvert.org/entrouvert/wcs/commit/60f2e07189f4d9a1737cd7ed4515a9699e4e0a33
pducroquet force-pushed wip/85622-optimize-action-global-timeout from b757531caf to 3d4c533df9 2024-01-14 16:34:11 +01:00 Compare
pducroquet force-pushed wip/85622-optimize-action-global-timeout from 3d4c533df9 to b4274b60ed 2024-01-14 16:43:47 +01:00 Compare
pducroquet requested review from fpeters 2024-01-14 17:03:57 +01:00
fpeters requested changes 2024-01-15 07:18:39 +01:00
fpeters left a comment
Owner
Copy Link

Attention ta série de commits est sur ton adresse privée; tu pourrais ajuster ça en squashant le tout ?

Attention ta série de commits est sur ton adresse privée; tu pourrais ajuster ça en squashant le tout ?
pducroquet force-pushed wip/85622-optimize-action-global-timeout from 997dbffd79 to 6cb2f0afef 2024-01-15 08:56:20 +01:00 Compare
pducroquet commented 2024-01-15 08:57:21 +01:00
Author
Owner
Copy Link

Attention ta série de commits est sur ton adresse privée; tu pourrais ajuster ça en squashant le tout ?

C'est fait, merci, j'avais pas fait gaffe que mon clone était mal configuré sur mon PC fixe.

> Attention ta série de commits est sur ton adresse privée; tu pourrais ajuster ça en squashant le tout ? C'est fait, merci, j'avais pas fait gaffe que mon clone était mal configuré sur mon PC fixe.
pducroquet requested review from fpeters 2024-01-15 09:27:49 +01:00
fpeters approved these changes 2024-01-15 11:28:57 +01:00
pducroquet merged commit 6cb2f0afef into main 2024-01-15 11:41:34 +01:00
pducroquet deleted branch wip/85622-optimize-action-global-timeout 2024-01-15 11:41:34 +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#1022
No description provided.
Delete Branch "wip/85622-optimize-action-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?