Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Réduit la requête de la topbar #3282

Merged
merged 2 commits into from
Jan 9, 2016

Conversation

artragis
Copy link
Member

@artragis artragis commented Jan 8, 2016

Q R
Correction de bugs ? [non]
Nouvelle Fonctionnalité ? [non]
Tickets (issues) concernés []

Notre topbar prenant exemple sur maven (la moitié d'internet tout ça...) j'ai donc modifié l'enchaînement de base de l'ORM pour que ça soit SQL qui fasse les calculs et qu'on se tappe pas les 10K tags et quelques à chaque fois.

@artragis
Copy link
Member Author

artragis commented Jan 8, 2016

bon comme c'est que du pep8, QA can start @SpaceFox le temps que je fasse le rebase nécessaire.

@DevHugo
Copy link
Contributor

DevHugo commented Jan 8, 2016

Je crois que de mémoire la gestion des tags était foireuse, ils était triés par id, j'avais fait une pr, y'a super longtemps et j'avais eu la flemme de rédiger la documentation donc c'était jamais passé et on avait fermé

@artragis
Copy link
Member Author

artragis commented Jan 8, 2016

non ils n'étaient pas triés par ID. on faisait notrecompteur nous même
en pyton là

Le 08/01/2016 22:05, Hugo Courtecuisse a écrit :

Je crois que de mémoire la gestion des tags était foireuse, ils était
triés par id, j'avais fait une pr, y'a super longtemps et j'avais eu
la flemme de rédiger la documentation donc c'était jamais passé et on
avait fermé


Reply to this email directly or view it on GitHub
#3282 (comment).

@DevHugo
Copy link
Contributor

DevHugo commented Jan 8, 2016

Oui, je sais ^^ . La méthode était buggé de mémoire =). Donc tu as résolu un bug en même temps.

Regarde: #2572 et #2573.

@SpaceFox
Copy link
Contributor

SpaceFox commented Jan 8, 2016

QA KO : ça n'affiche plus les tag, ce qui est logique puisque ta requête ne renvoie qu'une liste de couples (id tag, nombre d'apparitions).

D'autre part, je me demande si ça ne vaut pas le coup de mettre du cache sur cette requête, puisque quoiqu'on fasse elle sera longue.

Enfin je trouve la requête générée bizarrement compliquée, à la main on devrait obtenir quelque chose comme ça :

SELECT 
    utils_tag.*
FROM
    forum_topic
        INNER JOIN
    forum_topic_tags ON forum_topic_tags.topic_id = forum_topic.id
        INNER JOIN
    utils_tag ON forum_topic_tags.tag_id = utils_tag.id
WHERE
    `forum_topic`.`forum_id` IN (1 , 16,
        2,
        17,
        3,
        18,
        19,
        4,
        13,
        20,
        23,
        21,
        22,
        24,
        25)
GROUP BY forum_topic_tags.tag_id
ORDER BY COUNT(*) DESC
LIMIT 5;

@SpaceFox
Copy link
Contributor

SpaceFox commented Jan 8, 2016

Ça fonctionne, mais je trouve le bidule généré par l'ORM un peu lourdingue :

SELECT DISTINCT
    `forum_topic_tags`.`tag_id`,
    COUNT(`forum_topic_tags`.`tag_id`) AS `nb_tags`
FROM
    `forum_topic`
        LEFT OUTER JOIN
    `forum_topic_tags` ON (`forum_topic`.`id` = `forum_topic_tags`.`topic_id`)
        INNER JOIN
    `forum_topic_tags` T5 ON (`forum_topic`.`id` = T5.`topic_id`)
WHERE
    (`forum_topic`.`forum_id` IN (1 , 16,
        2,
        17,
        3,
        18,
        19,
        4,
        13,
        20,
        23,
        21,
        22,
        24,
        25)
        AND T5.`tag_id` IS NOT NULL)
GROUP BY `forum_topic_tags`.`tag_id`
ORDER BY `nb_tags` DESC
LIMIT 5;
SELECT 
    `utils_tag`.`id`, `utils_tag`.`title`, `utils_tag`.`slug`
FROM
    `utils_tag`
WHERE
    `utils_tag`.`id` IN (218 , 40, 5, 48, 6);

Je ne connais pas les contraintes de l'ORM Django, mais est-ce techniquement possible de transformer ces 2 requêtes en quelque chose comme ce que je propose au-dessus (et qui en plus est plus rapide) ?

@artragis
Copy link
Member Author

artragis commented Jan 9, 2016

ça devrait être bon.

@artragis
Copy link
Member Author

artragis commented Jan 9, 2016

avant que je squash, tu peux vérifier que tout fonctionne?

@artragis
Copy link
Member Author

artragis commented Jan 9, 2016

@gustavi c'est pas une évolution en soit, et au vu des problèmes de perfs que ça pose, ça tient même plus du bug.

@SpaceFox je squash dès que tu as (in)validé.

@gustavi
Copy link
Contributor

gustavi commented Jan 9, 2016

@artragis OK np, c'est surtout pour avoir des jolies générations de release :p

@gustavi gustavi added S-BUG Corrige un problème and removed Evolution labels Jan 9, 2016
@artragis
Copy link
Member Author

artragis commented Jan 9, 2016

oui, c'est pour ça que je précise.

Le 09/01/2016 18:32, Laville Augustin a écrit :

@artragis https://github.com/artragis OK np, c'est surtout pour
avoir des jolies générations de release :p


Reply to this email directly or view it on GitHub
#3282 (comment).

@SpaceFox
Copy link
Contributor

SpaceFox commented Jan 9, 2016

Ça ne fonctionne pas : la requête générée est très bizarre et ne renvoie pas les bonnes données, ce qui fait que les tags ne s'affichent pas.

SELECT 
    `forum_topic_tags`.`tag_id`,
    COUNT(`forum_topic_tags`.`tag_id`) AS `nb_tags`
FROM
    `forum_topic`
        INNER JOIN
    `forum_topic_tags` ON (`forum_topic`.`id` = `forum_topic_tags`.`topic_id`)
WHERE
    (`forum_topic`.`forum_id` IN (1 , 16, 2, 17, 3, 18, 19, 20, 23, 21, 22, 24)
        AND `forum_topic_tags`.`tag_id` IS NOT NULL)
GROUP BY `forum_topic`.`id` , `forum_topic`.`title` , `forum_topic`.`subtitle` , `forum_topic`.`forum_id` , `forum_topic`.`author_id` , `forum_topic`.`last_message_id` , `forum_topic`.`pubdate` , `forum_topic`.`is_solved` , `forum_topic`.`is_locked` , `forum_topic`.`is_sticky` , `forum_topic`.`key` , `forum_topic_tags`.`tag_id`
ORDER BY `nb_tags` DESC
LIMIT 5

@artragis
Copy link
Member Author

artragis commented Jan 9, 2016

Bon, je fais un dernier essai. Si ça marche pas, je revient à la
proposition initiale qui fait certes deux requêtes mais qui corrige déjà
le problème de perfs.

Le 09/01/2016 18:44, SpaceFox a écrit :

Ça ne fonctionne pas : la requête générée est très bizarre et ne
renvoie pas les bonnes données, ce qui fait que les tags ne
s'affichent pas.

SELECT
forum_topic_tags.tag_id,
COUNT(forum_topic_tags.tag_id)AS nb_tags
FROM
forum_topic
INNER JOIN
forum_topic_tags ON (forum_topic.id = forum_topic_tags.topic_id)
WHERE
(forum_topic.forum_id IN (1 ,16,2,17,3,18,19,20,23,21,22,24)
AND forum_topic_tags.tag_id IS NOT NULL)
GROUP BY forum_topic.id ,forum_topic.title ,forum_topic.subtitle ,forum_topic.forum_id ,forum_topic.author_id ,forum_topic.last_message_id ,forum_topic.pubdate ,forum_topic.is_solved ,forum_topic.is_locked ,forum_topic.is_sticky ,forum_topic.key ,forum_topic_tags.tag_id
ORDER BY nb_tags DESC
LIMIT 5


Reply to this email directly or view it on GitHub
#3282 (comment).

@SpaceFox
Copy link
Contributor

SpaceFox commented Jan 9, 2016

Je crains qu'il ne faille revenir à la solution à 2 requêtes. Au pire, on devrait pouvoir coller du cache sur cette partie du menu.

@artragis
Copy link
Member Author

artragis commented Jan 9, 2016

plus tard le cache :p

Le 09/01/2016 19:47, SpaceFox a écrit :

Je crains qu'il ne faille revenir à la solution à 2 requêtes. Au pire,
on devrait pouvoir coller du cache sur cette partie du menu.


Reply to this email directly or view it on GitHub
#3282 (comment).

@SpaceFox
Copy link
Contributor

SpaceFox commented Jan 9, 2016

Oui bien sûr plus tard :)

@SpaceFox
Copy link
Contributor

SpaceFox commented Jan 9, 2016

QA OK, je merge, merci !

SpaceFox added a commit that referenced this pull request Jan 9, 2016
@SpaceFox SpaceFox merged commit 0003550 into zestedesavoir:dev Jan 9, 2016
@SpaceFox SpaceFox added this to the Version de développement milestone Jan 9, 2016
@gustavi
Copy link
Contributor

gustavi commented Jan 19, 2016

J'ai une question assez urgente @artragis et @SpaceFox : pourquoi import itertools a été supprimé alors qu'on s'en sert encore plus bas (ligne 90) ?

@artragis
Copy link
Member Author

hain?

@@ -1,15 +1,13 @@
# coding: utf-8

from collections import OrderedDict
import itertools
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ça @artragis

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oui mais justement itertools n'est plus utilisé là.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, ça vient du fait que j'ai modifié ça dans une ZEP. Je vais adapter le code. Désolé du dérangement.

@artragis
Copy link
Member Author

ligne 90 de quel fichier?

Le 19 janvier 2016 à 11:25, Laville Augustin notifications@github.com a
écrit :

J'ai une question assez urgente @artragis https://github.com/artragis
et @SpaceFox https://github.com/SpaceFox : pourquoi import itertools a
été supprimé alors qu'on s'en sert encore plus bas (ligne 90) ?


Reply to this email directly or view it on GitHub
#3282 (comment)
.

@artragis artragis deleted the fix_topbar_request branch April 11, 2016 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-BUG Corrige un problème
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants