-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: WB-3666, optimize voluminous shares request #567
fix: WB-3666, optimize voluminous shares request #567
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK pour moi, à part le onFailure inutile à moins qu'il soit là exprès, mais dans ce cas ajouter un commentaire explicatif ?
.collect(Collectors.toList())) | ||
.onFailure(th -> { | ||
log.error("Failed fetching visibles", th); | ||
promise.fail("Failed fetching visibles : " + th.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ce cas est traité volontairement ?
Car UserUtils.findVisibles
n'échoue jamais : il renvoie un tableau vide et génère un log en cas d'erreur.
Du coup cette ligne de code (ce onFailure en fait) ne sera jamais atteinte.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bien vu, je retire le onFailure
break; | ||
|
||
// Parallelizing the process of fetching the visibles | ||
final int partitionSize = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il faudrait plutôt réussir à récupérer ça de la conf. Pour que ce soit transverse on pourrait s'appuyer sur la conf que passe déjà Starter
@@ -218,7 +219,7 @@ public static void findVisibles(EventBus eb, String userId, String customReturn, | |||
m.put("additionnalParams", additionnalParams); | |||
} | |||
m.put("userId", userId); | |||
eb.request(COMMUNICATION_USERS, m, new Handler<AsyncResult<Message<JsonArray>>>() { | |||
eb.request(COMMUNICATION_USERS, m, new DeliveryOptions().setSendTimeout(60000), new Handler<AsyncResult<Message<JsonArray>>>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cette valeur aussi si on peut la reprendre de la conf ce serait super
* fix: WB-3666, optimize voluminous shares * fix: WB-3666, fix after review * fix: WB-3666, shares optims parameterizable
* fix: WB-3666, optimize voluminous shares * fix: WB-3666, fix after review * fix: WB-3666, shares optims parameterizable
* fix: WB-3666, optimize voluminous shares * fix: WB-3666, fix after review * fix: WB-3666, shares optims parameterizable
* fix: WB-3666, optimize voluminous shares * fix: WB-3666, fix after review * fix: WB-3666, shares optims parameterizable
Description
Lorsque les favoris de partage contiennent un nombre important d'utilisateurs, la requête de recherche des utilisateurs visibles peut atteindre un timeout.
Actions réalisées :
Avec ces changements on parvient à répondre dans cas reproductible en recette-release : le partage à un favoris contenant plus de 4000 utilisateurs.
Fixes
https://edifice-community.atlassian.net/browse/WB-3666
Type of change
Please check options that are relevant.
Which packages changed?
Please check the name of the package you changed
Tests
Tests manuels directement sur recette-release (environnement pour lequel le bug était reproductible)
Reminder
Security flaws
Performance impacts (think bulk !)
Unit tests were replayed
Unit tests were added and/or changed
I have updated the reminder for the version including my modifications
All done ! 😃