router: add callro methods based on node zone #391

eugenepaniot · 2023-01-10T13:12:14Z

Pick a replica according prefered zone (highest priority replica zone) in round-robin manner.

Current callr* calls use only first replica. Current callbr* calls use replicas in round-robin manner only, without zonal preferences that leads to extra cross zonal traffic (that costs some pennies) and higher latency.

Current PR suggests two new methods and new load-balancing (replica selection) algorithm:

callbzro - Similar to existing callbro - prefer, but not limit, Inner zone balanced method with round-robin;
callbzre - Similar to existing callbre - but prefer, but not limit, Inner zone balanced method with round-robin;

The algorithm is quite simple but useful, it resets round-robin cursor balance_i into the main position (1 - highest priority replica) when reches the last replica in the same zone. It is possible as all replicas already sorted in the priority_list by weight(actually relies on replica zone and zone_weights).

Serpentian · 2023-01-10T16:07:30Z

Thanks for the patch! Looks great! Please get acquainted with code review procedure and make sure your PR comply with our styling rules. For example:

A patch should include tests for the new features;
The title of the commit should be router: add callro methods based on node zone. The commit message should explain why and what you did;
We don't usually use comments after a line of code.

Serpentian · 2023-01-10T16:54:42Z

vshard/router/init.lua

@@ -710,6 +718,10 @@ local function router_callbre(router, bucket_id, ...)
    return router_call_impl(router, bucket_id, 'read', true, true, ...)
 end

+local function router_callbzre(router, bucket_id, ...)
+    return router_call_impl(router, bucket_id, 'read', true, 2, ...)


If we want to introduce such feature, we should probably make API more explicit. Using 2 as the value for balance doesn't seem too good. A suppose, the better solution would be to make balance a string, and if its value is something like prefer_zone, balance replicas in a way, you propose. In all other cases of balance, simple rr can be used

Updated, thank you.

eugenepaniot · 2023-02-15T14:38:21Z

Sorry, very busy now to implement tests. Will try to do it soon. It will be very helpful if someone from QA Team helps with it. Thank you.

Gerold103

Thanks for the patch! See some comments below. You also need to add tests. We don't have a special QA team for writing them. Authors of patches do this. Without tests merging this PR might take unknown time, because we would need to write them ourselves and that is complicated, since this feature is not planned anywhere. How to write them - look at examples in test/ folder. You need the new sub-folders ending with -luatest in there.

Gerold103 · 2023-03-14T22:05:22Z

vshard/replicaset.lua

            local i = #replicaset.priority_list
            while i > 0 do
                r = replicaset_balance_replica(replicaset)
                i = i - 1
                if r:is_connected() and (not prefer_replica or r ~= master) and
-                   replica_check_backoff(r, now) then
+                    replica_check_backoff(r, now)
+                then


Please, revert unnecessary changes like these. These 3 lines didn't have to change at all. Besides, we never put then on a separate line. Lets keep it consistent with our code style.

Gerold103 · 2023-03-14T22:05:52Z

vshard/replicaset.lua

@@ -577,13 +577,33 @@ local function replicaset_template_multicallro(prefer_replica, balance)
    local function pick_next_replica(replicaset, now)
        local r
        local master = replicaset.master
+


Please, drop not needed empty lines.

Gerold103 · 2023-03-14T22:11:41Z

vshard/replicaset.lua

+                    -- Pick a replica according prefered zone (highest priority replica zone) in round-robin manner
+                    if balance == "prefer_zone" and prefered_zone then
+                        local cbi = replicaset.balance_i
+                        local nr = replicaset_balance_replica(replicaset)
+
+                        if prefered_zone and r.zone and r.zone == prefered_zone
+                            and nr.zone and nr.zone ~= prefered_zone
+                            and (not prefer_replica or nr ~= master)
+                        then
+                            -- Reset cursor to the main position if next replica is in different zone.
+                            replicaset.balance_i = 1
+                        else
+                            -- Restore rr-cursor position.
+                            replicaset.balance_i = cbi
+                        end
+                    end
+


You should strive to keep the code indentation to the minimum. For example, here it is easily achievable by just handling simple cases first and doing early return. Then you can handle the zone balancing with -4 spaces indentation:

if r:is_connected() and (not prefer_replica or r ~= master) and - replica_check_backoff(r, now) - then + replica_check_backoff(r, now) then + if balance ~= 'prefered_zone' or not prefered_zone then + return r + end -- Pick a replica according prefered zone (highest priority replica zone) in round-robin manner - if balance == "prefer_zone" and prefered_zone then - local cbi = replicaset.balance_i - local nr = replicaset_balance_replica(replicaset) + local cbi = replicaset.balance_i + local nr = replicaset_balance_replica(replicaset) - if prefered_zone and r.zone and r.zone == prefered_zone - and nr.zone and nr.zone ~= prefered_zone - and (not prefer_replica or nr ~= master) - then - -- Reset cursor to the main position if next replica is in different zone. - replicaset.balance_i = 1 - else - -- Restore rr-cursor position. - replicaset.balance_i = cbi - end + if prefered_zone and r.zone and r.zone == prefered_zone + and nr.zone and nr.zone ~= prefered_zone + and (not prefer_replica or nr ~= master) + then + -- Reset cursor to the main position if next replica is in different zone. + replicaset.balance_i = 1 + else + -- Restore rr-cursor position. + replicaset.balance_i = cbi end -

Gerold103 · 2023-03-14T22:15:24Z

vshard/replicaset.lua

-                   replica_check_backoff(r, now) then
+                    replica_check_backoff(r, now)
+                then
+                    -- Pick a replica according prefered zone (highest priority replica zone) in round-robin manner


Please, keep line width limit at 80 symbols.

Gerold103 · 2023-03-14T22:15:52Z

vshard/replicaset.lua

+                        local nr = replicaset_balance_replica(replicaset)
+
+                        if prefered_zone and r.zone and r.zone == prefered_zone
+                            and nr.zone and nr.zone ~= prefered_zone


No need to check r.zone and r.zone == .... You can just do r.zone == .... nil is comparable with other types just fine.

Gerold103 · 2023-03-14T22:26:51Z

vshard/replicaset.lua

+                            and nr.zone and nr.zone ~= prefered_zone
+                            and (not prefer_replica or nr ~= master)
+                        then
+                            -- Reset cursor to the main position if next replica is in different zone.


Replicas are not sorted by zones. Hence if in the replica list the first one is in zone1, then one in zone2, and others in zone1, your code will skip the tail-replicas always. That doesn't look good.

There are several solutions - don't touch balance_i. Simply skip replicas from other zones manually. Or sort the replicas by zones in the replica list. Or add a key-value dictionary to each replicaset object, where key = zone name, value = replica list in this zone.

I think it might be too complicated to do the last 2 options, so I suggest to just skip not fitting replicas during the iteration. It seems this code solves your problem and is multiple times shorter:

@@ -577,33 +577,18 @@ local function replicaset_template_multicallro(prefer_replica, balance) local function pick_next_replica(replicaset, now) local r local master = replicaset.master - if balance then - local prefered_zone = replicaset.priority_list[1].zone + local prefered_zone + if balance == 'prefered_zone' then + prefered_zone = replicaset.priority_list[1].zone + end local i = #replicaset.priority_list while i > 0 do r = replicaset_balance_replica(replicaset) i = i - 1 if r:is_connected() and (not prefer_replica or r ~= master) and - replica_check_backoff(r, now) - then - -- Pick a replica according prefered zone (highest priority replica zone) in round-robin manner - if balance == "prefer_zone" and prefered_zone then - local cbi = replicaset.balance_i - local nr = replicaset_balance_replica(replicaset) - - if prefered_zone and r.zone and r.zone == prefered_zone - and nr.zone and nr.zone ~= prefered_zone - and (not prefer_replica or nr ~= master) - then - -- Reset cursor to the main position if next replica is in different zone. - replicaset.balance_i = 1 - else - -- Restore rr-cursor position. - replicaset.balance_i = cbi - end - end - + replica_check_backoff(r, now) and + (not prefered_zone or prefered_zone == r.zone) then return r end end

Of course, we would need tests to be sure.

Gerold103 · 2023-03-14T22:27:08Z

vshard/replicaset.lua

@@ -1235,6 +1258,7 @@ local function buildall(sharding_cfg)
        local function replica_cmp_weight(r1, r2)
            -- Master has priority over replicas with the same
            -- weight.
+


Please, drop this line.

Gerold103 · 2023-03-14T22:29:27Z

vshard/router/init.lua

@@ -576,11 +576,15 @@ local function router_call_impl(router, bucket_id, mode, prefer_replica,
    local call
    if mode == 'read' then
        if prefer_replica then
-            if balance then
+            if balance == "prefer_zone" then


Please, change 'balance' to numeric IDs. String comparison in Lua not always works for constant time and might degrade to linear time. Like this:

@@ -65,6 +65,10 @@ local fiber_yield = fiber.yield local fiber_cond_wait = util.fiber_cond_wait local gsc = util.generate_self_checker +local BALANCE_NONE = 0 +local BALANCE_RR = 1 +local BALANCE_BEST_ZONE_RR = 2 + -- -- on_connect() trigger for net.box -- @@ -578,7 +582,7 @@ local function replicaset_template_multicallro(prefer_replica, balance) local r local master = replicaset.master - if balance then + if balance ~= BALANCE_NONE then local prefered_zone = replicaset.priority_list[1].zone local i = #replicaset.priority_list while i > 0 do @@ -588,7 +592,7 @@ local function replicaset_template_multicallro(prefer_replica, balance) replica_check_backoff(r, now) then -- Pick a replica according prefered zone (highest priority replica zone) in round-robin manner - if balance == "prefer_zone" and prefered_zone then + if balance == BALANCE_BEST_ZONE_RR and prefered_zone then local cbi = replicaset.balance_i local nr = replicaset_balance_replica(replicaset) @@ -1009,12 +1013,12 @@ local replicaset_mt = { wait_connected_all = replicaset_wait_connected_all, call = replicaset_master_call; callrw = replicaset_master_call; - callro = replicaset_template_multicallro(false, false); - callbro = replicaset_template_multicallro(false, true); - callbzro = replicaset_template_multicallro(false, "prefer_zone"); - callre = replicaset_template_multicallro(true, false); - callbre = replicaset_template_multicallro(true, true); - callbzre = replicaset_template_multicallro(true, "prefer_zone"); + callro = replicaset_template_multicallro(false, BALANCE_NONE); + callbro = replicaset_template_multicallro(false, BALANCE_RR); + callbzro = replicaset_template_multicallro(false, BALANCE_BEST_ZONE_RR); + callre = replicaset_template_multicallro(true, BALANCE_NONE); + callbre = replicaset_template_multicallro(true, BALANCE_RR); + callbzre = replicaset_template_multicallro(true, BALANCE_BEST_ZONE_RR); map_call = replicaset_map_call, update_master = replicaset_update_master, };

Thank you for the comments. Will check it.

Add callro methods based on node zone

a636535

Serpentian reviewed Jan 10, 2023

View reviewed changes

Serpentian requested a review from Gerold103 January 10, 2023 16:55

eugenepaniot changed the title ~~Pick a replica according prefered zone (highest priority replica zone) in round-robin manner.~~ router: pick a replica according prefered zone in round-robin manner Jan 11, 2023

Update comments

b3656e7

eugenepaniot changed the title ~~router: pick a replica according prefered zone in round-robin manner~~ router: add callro methods based on node zone Jan 17, 2023

eugenepaniot added 2 commits January 22, 2023 16:51

Use strings as load-balancing algorithm identificator

774dfb2

Update test/router/router.result add new funct

d22797c

Gerold103 reviewed Mar 14, 2023

View reviewed changes

kyukhin marked this pull request as draft October 20, 2023 07:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

router: add callro methods based on node zone #391

router: add callro methods based on node zone #391

eugenepaniot commented Jan 10, 2023 •

edited

Loading

Serpentian commented Jan 10, 2023 •

edited

Loading

Serpentian Jan 10, 2023

eugenepaniot Feb 15, 2023

eugenepaniot commented Feb 15, 2023

Gerold103 left a comment

Gerold103 Mar 14, 2023

Gerold103 Mar 14, 2023

Gerold103 Mar 14, 2023

Gerold103 Mar 14, 2023

Gerold103 Mar 14, 2023

Gerold103 Mar 14, 2023

Gerold103 Mar 14, 2023

Gerold103 Mar 14, 2023

eugenepaniot Mar 17, 2023

router: add callro methods based on node zone #391

Are you sure you want to change the base?

router: add callro methods based on node zone #391

Conversation

eugenepaniot commented Jan 10, 2023 • edited Loading

Serpentian commented Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugenepaniot commented Feb 15, 2023

Gerold103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugenepaniot commented Jan 10, 2023 •

edited

Loading

Serpentian commented Jan 10, 2023 •

edited

Loading