-
Notifications
You must be signed in to change notification settings - Fork 77
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
feature: Round Robin with optional random start #26
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.
Yeah, start randomly should be better, thanks for your work :)
README.markdown
Outdated
@@ -126,7 +126,7 @@ Both `resty.chash` and `resty.roundrobin` have the same apis. | |||
|
|||
new | |||
--- | |||
**syntax:** `obj, err = class.new(nodes)` | |||
**syntax:** `obj, err = class.new(nodes, random_start?)` |
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.
I think we can always make it start randomly.
lib/resty/roundrobin.lua
Outdated
@@ -46,18 +47,40 @@ local function get_gcd(nodes) | |||
return only_key, gcd, max_weight | |||
end | |||
|
|||
local function get_random_node_id(nodes) | |||
local count = 0 |
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.
style: need four space as indent.
@doujiang24 thanks for quick review! I addressed your comments. |
lib/resty/roundrobin.lua
Outdated
local function get_random_node_id(nodes) | ||
local count = 0 | ||
for _, _ in pairs(nodes) do | ||
count = count + 1 |
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.
style: need four space too :)
lib/resty/roundrobin.lua
Outdated
local random_index = math_random(count) | ||
|
||
for _ = 1, random_index do | ||
id = next(nodes, id) |
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.
ditto.
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.
oops, I missed them - fixed now
Is there anything else I can do to get this merged and released? |
@doujiang24 any plan here? |
@doujiang24 when are you planning on releasing this? |
@ElvinEfendi |
@doujiang24 thanks! Does it get automatically published to Luarocks? Or do you suggest using OPM? |
not yet.
OPM doesn't support this library yet since it only supports pure Lua library now. |
It's sometimes useful to have Round Robin algorithm to start with a random node in the given list instead of always starting with the first one. Imagine you have 50 replicas on Nginx and in each replica 16 workers. That means all 50*16 requests will end up in the first server.
For another real world use case please refer to the thread in kubernetes/ingress-nginx#4023 (comment).
This PR adds an optional flag to Round Robin initializer and if the flag is set it picks
last_id
at random instead of setting it to `nil.