-
Notifications
You must be signed in to change notification settings - Fork 5
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
Make the load balancer immediately aware of the number of playing players #28
Make the load balancer immediately aware of the number of playing players #28
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.
On first inspection this seems to do what you want it to do, will merge when you have the graphs of the test results.
Only concern I have is when a client gets connected to a node with other clients connected to it, this is not kept into account with this change
By testing I mean using it in a small environment to see if it is calculating penalties as expected. That graph has been taken from production, and it would be a while before this change lands there. |
That makes sense, if the test env gives the wanted results it's fine with me |
src/main/kotlin/dev/arbjerg/lavalink/internal/loadbalancing/Penalties.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: duncte123 <contact@duncte123.me>
14dfdc4
to
e25ebaf
Compare
Am I ok to merge this? |
I believe so? Do you have reason to be in doubt? |
Then I will merge it! |
This makes the load balancer more responsive to quick increases in player count, as it will not rely on outdated player data.
The current behavior leads to a sawtooth-esque patterns, like seen with this one node:
The original V1 client used this: https://github.com/freyacodes/Lavalink-Client/blob/master/src/main/java/lavalink/client/io/LavalinkLoadBalancer.java#L112
I have not tested this code yet.