entrouvert
/
django-mellon
16
0
Fork 0
Code Issues Pull Requests Releases Activity

Ne pas émettre d'erreur de mise à jour des métadonnées sur un cache froid (#84933) #11

Merged
bdauvergne merged 1 commits from wip/84933-Traces-concernant-l-impossibilit into main 2024-01-16 12:44:54 +01:00
Conversation 0 Commits 1 Files Changed 3 +157 -132
bdauvergne commented 2023-12-19 17:59:46 +01:00
Owner
Copy Link
No description provided.
bdauvergne added 2 commits 2023-12-19 17:59:47 +01:00
cc1d275e40 adapters: do not log errors on cold cache (#84933) 
Only log errors if the cache is older than 24 hours.
bdauvergne force-pushed wip/84933-Traces-concernant-l-impossibilit from cc1d275e40 to 4900475e70 2023-12-19 18:01:18 +01:00 Compare
pmarillonnet reviewed 2024-01-11 09:42:50 +01:00
pmarillonnet left a comment
Owner
Copy Link

Hop, un premier bout de relecture, comme dit dans mes commentaires il me manque des éléments pour comprendre, je passe à côté d’un truc à la relecture :/

Hop, un premier bout de relecture, comme dit dans mes commentaires il me manque des éléments pour comprendre, je passe à côté d’un truc à la relecture :/
mellon/adapters.py Outdated
@ -104,4 +109,3 @@
logger.error(
'mellon: metadata path %s : entityID changed %r != %r', path, entity_id, idp['ENTITY_ID']
)
del idp['ENTITY_ID']
pmarillonnet commented 2024-01-11 09:27:52 +01:00
Owner
Copy Link

Pourquoi laisse-t-on de côté cette suppression dans cette nouvelle version du code ?

Pourquoi laisse-t-on de côté cette suppression dans cette nouvelle version du code ?
bdauvergne commented 2024-01-11 11:04:21 +01:00
Author
Owner
Copy Link

Je vais plutôt mettre un raise ici, c'est vraiment pas normal de voir changer l'entity_id.

Je vais plutôt mettre un raise ici, c'est vraiment pas normal de voir changer l'entity_id.
bdauvergne marked this conversation as resolved
mellon/adapters.py Outdated
@ -91,2 +95,2 @@
if last_update == 0 or mtime >= last_update:
idp['METADATA_PATH_LAST_UPDATE'] = time.time()
if mtime > last_mtime or metadata_last_update <= mtime:
pmarillonnet commented 2024-01-11 09:39:11 +01:00
Owner
Copy Link

Je ne capte pas du tout ce qu’apporte cette deuxième comparaison, pour en avoir une avec idp['METADATA_PATH_MTIME'] et une autre avec idp['METADATA_LAST_UPDATE']. Je passe à côté d’un truc, pas compris du tout pas ce que ça apporte :/

Je pense qu’il faudrait documenter ces deux METADATA_* (parce que je ne comprends pas, par exemple, les cas où la date de dernière modification locale ne correspondrait pas à une modification des métadonnées. J’étais dans l’idée qu’elles ne sont ré-écrites localement que lorsqu’elles ont changé, non ?)

Je ne capte pas du tout ce qu’apporte cette deuxième comparaison, pour en avoir une avec `idp['METADATA_PATH_MTIME']` et une autre avec `idp['METADATA_LAST_UPDATE']`. Je passe à côté d’un truc, pas compris du tout pas ce que ça apporte :/ Je pense qu’il faudrait documenter ces deux `METADATA_*` (parce que je ne comprends pas, par exemple, les cas où la date de dernière modification locale ne correspondrait pas à une modification des métadonnées. J’étais dans l’idée qu’elles ne sont ré-écrites localement que lorsqu’elles ont changé, non ?)
bdauvergne commented 2024-01-11 11:14:39 +01:00
Author
Owner
Copy Link

C'est une scorie du code intermédiaire, METADATA_PATH_MTIME n'existe même plus en fait (il n'est jamais posé dans idp). Branche à jour.

C'est une scorie du code intermédiaire, METADATA_PATH_MTIME n'existe même plus en fait (il n'est jamais posé dans idp). Branche à jour.
pmarillonnet commented 2024-01-16 10:43:16 +01:00
Owner
Copy Link

C'est une scorie du code intermédiaire, METADATA_PATH_MTIME n'existe même plus en fait (il n'est jamais posé dans idp). Branche à jour.

Ok, je reprends ma relecture.

> C'est une scorie du code intermédiaire, METADATA_PATH_MTIME n'existe même plus en fait (il n'est jamais posé dans idp). Branche à jour. Ok, je reprends ma relecture.
pmarillonnet marked this conversation as resolved
bdauvergne force-pushed wip/84933-Traces-concernant-l-impossibilit from 4900475e70 to 2af5808bc0 2024-01-11 11:13:51 +01:00 Compare
pmarillonnet requested review from pmarillonnet 2024-01-16 10:43:24 +01:00
bdauvergne removed review request for pmarillonnet 2024-01-16 10:44:46 +01:00
bdauvergne requested review from pmarillonnet 2024-01-16 10:46:19 +01:00
bdauvergne removed review request for pmarillonnet 2024-01-16 11:05:20 +01:00
bdauvergne requested review from pmarillonnet 2024-01-16 11:05:46 +01:00
pmarillonnet reviewed 2024-01-16 11:37:10 +01:00
pmarillonnet left a comment
Owner
Copy Link

Voilà, deuxième ronde et encore d’autres interrogations même si je commence à y voir plus clair.

Voilà, deuxième ronde et encore d’autres interrogations même si je commence à y voir plus clair.
mellon/adapters.py Outdated
@ -93,0 +86,4 @@
metadata_last_update = idp.get('METADATA_LAST_UPDATE', None)
if not idp.get('METADATA'):
pmarillonnet commented 2024-01-16 11:10:51 +01:00
Owner
Copy Link

Du détail, mais du code churn ici sur les versions successives et juste ça pourrait devenir

if not idp.get('METADATA') or not metadata_last_update:
    should_load = True
else:
Du détail, mais du code churn ici sur les versions successives et juste ça pourrait devenir ```python if not idp.get('METADATA') or not metadata_last_update: should_load = True else: ```
bdauvergne commented 2024-01-16 11:47:16 +01:00
Author
Owner
Copy Link

Je trouve plus clair de séparer les conditions sur deux lignes.

Je trouve plus clair de séparer les conditions sur deux lignes.
bdauvergne marked this conversation as resolved
mellon/adapters.py Outdated
@ -158,0 +167,4 @@
metadata_last_update
and 'METADATA' in idp
and (
(now - metadata_last_update) < metadata_cache_time / 2
pmarillonnet commented 2024-01-16 11:23:30 +01:00
Owner
Copy Link

Pourquoi c’est la moitié de la valeur qui se retrouve utilisée comme durée maximale d’utilisation du cache ?

Pourquoi c’est la moitié de la valeur qui se retrouve utilisée comme durée maximale d’utilisation du cache ?
bdauvergne commented 2024-01-16 11:49:37 +01:00
Author
Owner
Copy Link

Je vais virer le / 2.

Je vais virer le / 2.
bdauvergne marked this conversation as resolved
mellon/adapters.py Outdated
@ -180,1 +197,3 @@
del idp['ENTITY_ID']
return
if idp.get('METADATA') != response.text:
pmarillonnet commented 2024-01-16 11:27:38 +01:00
Owner
Copy Link

Ok peut-être un jour (et dans un autre ticket) comparer au sens XML et pas simplement le contenu pour éviter les ré-écritures inutiles (genre des ordres de balises ou d’attributs qui changent, sans aucune conséquence) ?

Ok peut-être un jour (et dans un autre ticket) comparer au sens XML et pas simplement le contenu pour éviter les ré-écritures inutiles (genre des ordres de balises ou d’attributs qui changent, sans aucune conséquence) ?
bdauvergne commented 2024-01-16 11:49:14 +01:00
Author
Owner
Copy Link

Ça n'arrive pas.

Ça n'arrive pas.
pmarillonnet marked this conversation as resolved
mellon/adapters.py Outdated
@ -235,0 +242,4 @@
if 'METADATA_PATH' in idp:
self.load_metadata_path(idp, i)
if 'METADATA_URL' in idp:
pmarillonnet commented 2024-01-16 11:33:59 +01:00
Owner
Copy Link

Peut-être un 'elif' ici pour éviter de potentiellement charger les métadonnées deux fois, et ceci en choisissant arbitrairement un ordre (genre une chemin prend le dessus sur une URL), avec petit warning sur une double définition des métadonnées par deux moyens différents ?

Peut-être un 'elif' ici pour éviter de potentiellement charger les métadonnées deux fois, et ceci en choisissant arbitrairement un ordre (genre une chemin prend le dessus sur une URL), avec petit warning sur une double définition des métadonnées par deux moyens différents ?
bdauvergne commented 2024-01-16 11:52:46 +01:00
Author
Owner
Copy Link

Ok pour le elif.

Ok pour le elif.
bdauvergne marked this conversation as resolved
mellon/adapters.py Outdated
@ -239,21 +251,28 @@ class DefaultAdapter:
if entity_id:
idp['ENTITY_ID'] = entity_id
if age and age > (24 * metadata_cache_time):
pmarillonnet commented 2024-01-16 11:36:27 +01:00
Owner
Copy Link

Y'a un truc louche ici. 10 lignes plus haut on comparaît directement age et metadata_cache_time, mais ici c’est age et 24 * metadata_cache_time qu’on compare. Ça a une tête d’erreur d’unité ici.

Y'a un truc louche ici. 10 lignes plus haut on comparaît directement `age` et `metadata_cache_time`, mais ici c’est `age` et `24 * metadata_cache_time` qu’on compare. Ça a une tête d’erreur d’unité ici.
bdauvergne commented 2024-01-16 11:55:01 +01:00
Author
Owner
Copy Link

Par défaut METADATA_CACHE_TIME est à 1h, j'ai décidé arbitrairement qu'on lèverait une erreur si le cache a plus de 24h tout simplement.

Par défaut METADATA_CACHE_TIME est à 1h, j'ai décidé arbitrairement qu'on lèverait une erreur si le cache a plus de 24h tout simplement.
pmarillonnet commented 2024-01-16 12:24:22 +01:00
Owner
Copy Link

Y'a un truc louche ici. 10 lignes plus haut on comparaît directement age et metadata_cache_time, mais ici c’est age et 24 * metadata_cache_time qu’on compare. Ça a une tête d’erreur d’unité ici.

Ah bon ok j’ai cru qu’on comparait des heures et des jours à un moment.

> Y'a un truc louche ici. 10 lignes plus haut on comparaît directement `age` et `metadata_cache_time`, mais ici c’est `age` et `24 * metadata_cache_time` qu’on compare. Ça a une tête d’erreur d’unité ici. Ah bon ok j’ai cru qu’on comparait des heures et des jours à un moment.
pmarillonnet marked this conversation as resolved
bdauvergne force-pushed wip/84933-Traces-concernant-l-impossibilit from 2af5808bc0 to ef3ebcf951 2024-01-16 11:55:51 +01:00 Compare
pmarillonnet approved these changes 2024-01-16 12:26:12 +01:00
bdauvergne requested review from nroche 2024-01-16 12:39:58 +01:00
bdauvergne removed review request for nroche 2024-01-16 12:40:05 +01:00
bdauvergne force-pushed wip/84933-Traces-concernant-l-impossibilit from ef3ebcf951 to af81da4954 2024-01-16 12:41:27 +01:00 Compare
bdauvergne merged commit af81da4954 into main 2024-01-16 12:44:54 +01:00
bdauvergne deleted branch wip/84933-Traces-concernant-l-impossibilit 2024-01-16 12:44:54 +01:00
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/django-mellon#11
No description provided.
Delete Branch "wip/84933-Traces-concernant-l-impossibilit"

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?