Optimisation des requêtes pour l'économie

[gtolontop]

Petit résumé de la PR

Optimise la persistance des soldes économie en supprimant les écritures DB à chaque modification et en regroupant la sauvegarde à l'arrêt de la feature.

Étape nécessaire afin que la PR soit finie

• Suivre le Code de Conduite
• Enlever tous les imports non utilisés
• Bien documenter la feature
• Fournir un profileur (non demandé pour cette PR)
• Avoir une milestone associée à la PR (2.5.0 à assigner)
• Valider tous les checks
• Tester et valider la feature/changement

Fixes #1332

Changements

• addBalance, withdrawBalance et setBalance modifient uniquement le cache en mémoire ;
• Feature.save() persiste tous les soldes avec ORMLite.callBatchTasks() lors de l'arrêt ;
• aucun autosave asynchrone, verrou synchronized ou SQL écrit à la main ;
• une lecture de solde inconnu renvoie 0 sans créer de compte dans le cache ;
• deux tests ciblés couvrent l'absence d'effet de bord à la lecture et la persistance mémoire vers DB au save.

Validation

• compileJava passe avec Java 25 sur la branche mise à jour avec master ;
• l'exécution locale des tests est actuellement bloquée avant compilation par JitPack (401 Unauthorized sur mockbukkit-v26.2:4.114.0).

[AxenoDev]

au lieu de sauvegarder tout les x temps, pourquoi tu save pas juste a la db au moment ou le plugin se desactive

[iambibi]

C'était marqué dans l'issue

[gtolontop]

au lieu de sauvegarder tout les x temps, pourquoi tu save pas juste a la db au moment ou le plugin se desactive

C’est déjà fait xd au save() de la feature, donc à la désactivation du plugin, saveAllBalances() flush les balances modifiées

L’autosave est volontaire en plus du shutdown save ça évite de perdre toutes les modifications depuis le démarrage si le serveur crash/kill ou si l’arrêt ne va pas jusqu’au bout. Et il ne sauvegarde pas tous les balances à chaque fois, seulement les UUID marqués dirty depuis la dernière sauvegarde :)

[AxenoDev]

Le serveur n'est pas cense etre kill ou crash, meme il ne sera jamais kill...

[iambibi]

Ben après il peut crash oui. Mais il est tjr éteint à 2h donc cv

[gtolontop]

Même si le serveur n’est pas censé crash il peut crash mdrrrrr l’autosave limite la perte si ça arrive et dans tous les cas le coût en perf est faible on ne save pas tous les comptes seulement les dirty en batch

[iambibi]

Tu appelles quoi un compte dirty?

[gtolontop]

Dirty = modifié depuis le dernier save DB
On garde l’UUID dans dirtyBalances et au prochain save on persiste seulement ces comptes

[AxenoDev]

Le probleme, est que la le lag a lieu lors de la premiere connexion des joeurs, c'est un moment que l'on avait fait avec 50bots de souvenir, et le lag venait du save a la db, donc la si je comprends bien, a un moment, le serveur va lag parcequ'il save toutes les donne dans la db a un moment x

[iambibi]

Fin et déjà pour qu'on valide le problème, il faudra que tu donnes un jar de ton plugin, on fait un teste avec 50-100 joueurs, et on attends jusqu'au moment où le scheduler s'exécute pour save la db, et on tentera

[AxenoDev]

Bon @gtolontop, si c'est pour faire du vibecoding sur le projet ce n'est pas la peine de venir faire une PR.

[Shoccapik]

Le satan qui se réveille avec le full vibecoding qui fait mal

[gtolontop]

Bon @gtolontop, si c'est pour faire du vibecoding sur le projet ce n'est pas la peine de venir faire une PR.

MDRRRRR, elle est vraiment niquel ma PR je vois pas le soucis + c'est pas du vibecoding mais j'avoue que mon code a une vibe très propre :)

Ta mal pris le faite que tu es tort ici ?

ou ici ?

[gtolontop]

Le satan qui se réveille avec le full vibecoding qui fait mal

Très propre se vibecoding en tous cas

[gtolontop]

Bon @gtolontop, si c'est pour faire du vibecoding sur le projet ce n'est pas la peine de venir faire une PR.

Dis-moi, qu'aurais-tu fait différemment ?

[AxenoDev]

Donc pourquoi il y a ecris CODEX dans ta PR ....
Ne me prends pas pour un con non plus

[AxenoDev]

Bon @gtolontop, si c'est pour faire du vibecoding sur le projet ce n'est pas la peine de venir faire une PR.

Dis-moi, qu'aurais-tu fait différemment ?

Honnetement, bcp de chose, mais si tu "ne vibecode pas" chaque developpeur fait differemment donc je n'aurais pas fait pareil que toi

[iambibi]

Bon @gtolontop, si c'est pour faire du vibecoding sur le projet ce n'est pas la peine de venir faire une PR.

MDRRRRR, elle est vraiment niquel ma PR je vois pas le soucis + c'est pas du vibecoding mais j'avoue que mon code a une vibe très propre :)

Ta mal pris le faite que tu es tort ici ?

ou ici ?

Tu mets tout sur le principe de la raison? Pourquoi ?
Tu as des choses à montrer?
Avoir tort n'est pas négatif le temps que la personne explique pourquoi elle a tort.

[AxenoDev]

https://spark.lucko.me/xOiSgERLyM avec 50joueurs @iambibi

[iambibi]

https://spark.lucko.me/xOiSgERLyM avec 50joueurs @iambibi

T'es sûr d'avoir attendu le save des économies ?

[AxenoDev]

j'ai attendu 5minutes

[iambibi]

Y'a pas de bugs? Les joueurs ont bien leur thune de save?

[gtolontop]

Y'a pas de bugs? Les joueurs ont bien leur thune de save?

Oui j'ai déjà tester xd

[ltuffery]

Mockito fera l’objet d’une autre PR, c’est ce que je voulais dire.

Concernant ta réflexion, tu dois la retirer : soit tu arrives à remplacer les tests avec l’API actuelle (ce qui serait idéal), soit tu trouves une autre approche.

[gtolontop]

C’est traité dans le commit juste au dessus

J’ai retiré la réflexion du test y'a plus de getDeclaredField, getDeclaredMethod, setAccessible les tests passent maintenant par une vraie DB H2/ORMLite. Pour garder la couverture du cas finalSave, j’utilise un trigger H2 qui modifie une balance pendant l’écriture DB, ce qui permet de tester le comportement sans accéder à l’état privé de EconomyManager voilaaaa

vous pouvez testez avec

./gradlew test --tests fr.openmc.core.features.economy.EconomyManagerDirtySaveTest --no-daemon --rerun-tasks

[iambibi]

Attente de review

[gtolontop]

[AxenoDev]

y'a beaucoup de test pour "rien", puis le fait d'ecrire en dur les requêtes sql j'vois pas le principe
puis pourquoi utiliser synchronize et pas utiliser les methodes de base de Bukkit ?

[iambibi]

y'a beaucoup de test pour "rien", puis le fait d'ecrire en dur les requêtes sql j'vois pas le principe

Fin les tests ne servent pas a rien, mais la actuellement sur le projet presque rien a des implémentations d'unit test. Et c'est vraiment inutile de faire des tests où tu commences à faire des codes hyper sombre.
Mais par contre oui gtol répond à ça :

puis pourquoi utiliser synchronize et pas utiliser les methodes de base de Bukkit ?

[AxenoDev]

j'voulais pas dire que les testes ne servent a rien, mais que dans le cas de ce projet, autant de testes servent a rien

[iambibi]

Fermeture de la PR dans 1 semaines si aucune réponse :

Raison aucune réponse et action aux messages depuis 1 semaines.

[iambibi]

dernière chance avant d'être décaler à la 2.5.0-beta-2, (avant 18h)

[gtolontop]

J'ai repris les trois points du dernier retour et poussé une version simplifiée, après merge de master.

• Sur les tests : je ne pense pas que les tests soient inutiles en soi, mais je suis d'accord que le scénario avec triggers H2 était trop complexe pour cette issue. Le fichier a été supprimé, ainsi que le test qui utilisait encore de la réflexion. Il reste seulement deux tests directement liés au changement.
• Le SQL écrit en dur servait uniquement à fabriquer une course dans ce test H2, pas au code de production. Comme ce test a été retiré, il n'y a plus de SQL manuel.
• Les synchronized servaient à protéger le cache contre l'autosave asynchrone. L'autosave étais pas nécessaire finalement donc je l'ai retiré. Les modifications restent maintenant en mémoire sur le flux Bukkit habituel, puis Feature.save() fait un seul batch ORMLite à l'arrêt. Il n'y a donc plus de synchronized.

La branche n'est plus en conflit avec master aussi

[iambibi]

J'ai repris les trois points du dernier retour et poussé une version simplifiée, après merge de master.

• Sur les tests : je ne pense pas que les tests soient inutiles en soi, mais je suis d'accord que le scénario avec triggers H2 était trop complexe pour cette issue. Le fichier a été supprimé, ainsi que le test qui utilisait encore de la réflexion. Il reste seulement deux tests directement liés au changement.
• Le SQL écrit en dur servait uniquement à fabriquer une course dans ce test H2, pas au code de production. Comme ce test a été retiré, il n'y a plus de SQL manuel.
• Les synchronized servaient à protéger le cache contre l'autosave asynchrone. L'autosave étais pas nécessaire finalement donc je l'ai retiré. Les modifications restent maintenant en mémoire sur le flux Bukkit habituel, puis Feature.save() fait un seul batch ORMLite à l'arrêt. Il n'y a donc plus de synchronized.

La branche n'est plus en conflit avec master aussi

C'est sur que c'est plus compréhensible et puis c'est sur que ça marche, le serveur s'éteint tout le temps à 2h, si y'a un crash il peut rollback de 24h, si y'a un crash

[iambibi]

Je retrouve pas ton message sur le statut des tests, je pense que c'est normal qui plante j'ai update en 26.2 mais mock bukkit n'était pas encore sorti en 26.2, j'irais voir

[iambibi]

MockBukkit/MockBukkit#1592
Ça bosse bien dessus.

[iambibi]

juste un petit if (bool) return a faire au lieu d'un scope (une question de préférence de ma part, je trouve que ça fait plus clean) prévients moi des que tu as fais le changement que je merge ta PR. tt le reste est niquel