router: make full map_callrw with split args

[mrForza]

This patch introduces a new way of map_callrw execution by which we can
pass some arguments to all storages and split buckets' arguments to those
storages that have at least one bucket of bucket_ids. To achieve this we
introduce a new string option - mode to map_callrw api.

Also we change the logic of router_ref_storage_all ref function. Firstly
we ref all storages and get back an amount of "moved" buckets according
to the previously built router's cache. Then if there are no "moved"
buckets we accumulate and check total amount of buckets on all storages
and finish map_callrw ref stage. Otherwise, if there are some "moved"
buckets we perform the second network hop by checking on which replicasets
do the remaining "moved" buckets reside on.

Closes #559

[Serpentian]

Well done, only one major comment (upgrade), we must address, other ones are nits and smth to think about)

[Serpentian]

Hmm, did you consider making it even more general: router_map_callrw_send and router_map_callrw_collect and also reuse them in the replicasets_map_reduce too? There we have almost the same code. Not a call to action, just something to think about.

[mrForza]

Yes, router_map_callrw_prepare/send/collect sounds better.

But I don't think that we can easily reuse these functions in replicasets_map_reduce because:

1. We need to change api of router_map_callrw_send by
   • changing router param into replicasets_all, as replicasets_map_reduce does not have router variable in it.
   • adding extra argument - return_raw
2. The logic of sending map stage in replicasets_map_reduce is more complex than logic of router_map_callrw_send. In last one we only do RPC with no arguments or with grouped_buckets[rs_id] and that's all. But in replicasets_map_reduce we need to dynamically change those arguments which should be passed in RPC. Before RPC we add bucket arguments to args and after RPC we delete them. The main problem here is that we can only easily add groupped buckets in args builder, but not delete, because the deletion is happened after RPC. If we want to do it in our new router_map_callrw_send function we need to complicate args_builder and use table.deepcopy (it can slow down perf).
3. We need to change api of router_map_callrw_collect by passing extra return_raw argument.
4. Also router_map_callrw_collect will be complicated as we should add two different ways of extracting results.

[Serpentian]

changing router param into replicasets_all, as replicasets_map_reduce does not have router variable in it.

1.1 Yes, and it will become replicasets_map_send, which is way cleaner, since you will explicitly pass the replicasets and not use some non obvious loigic to create the list of replicasets based on optional argument grouped_buckets (which won't be needed). You can return replicasets from the router_map_prepare.

adding extra argument - return_raw

1.2. Let's just pass the opts and add is_async explicitly inside the function. And call the funciton router_map_callrw_send_async. The name should always say, what the function does.

The logic of sending map stage in replicasets_map_reduce is more complex than logic of router_map_callrw_send.

2. Agree, see no other way than deepcopy.

We need to change api of router_map_callrw_collect by passing extra return_raw argument.
Also router_map_callrw_collect will be complicated as we should add two different ways of extracting results.

3. You can always just create the new function router_map_callrw_collect_raw and not pass the raw to it

Again, up to you, if you think, that the current variant is better, I'm ok

[kamenkremen]

Great patch! Left some minor comments below

[kamenkremen]

Isn't that equivalent to local rs_ids = grouped_buckets or replicasets_all?

[mrForza]

fixed

[kamenkremen]

Comment got cut out

[mrForza]

fixed

[kamenkremen]

futures is already either {}(because of line 820) or some table, so we don't need this

[mrForza]

fixed

[kamenkremen]

Nit: triple space in indentation

[mrForza]

fixed

[Serpentian]

I don't have any major comments, I think it's time to ask @Gerold103 for review, so that we can be sure, that there no major flaws, which I've missed

[Serpentian]

Should be in the first commit

[mrForza]

fixed

[Serpentian]

No, it's not

[Gerold103]

Thanks for working on this complex topic and being patient about comments 🙏.

[Serpentian]

Nit: I'd propose to specify the whole arg table. It's very difficult to parse

[mrForza]

fixed

[Gerold103]

I am so sorry, but I have to write more comments 😭🥲.

[Gerold103]

The problem is same as before - bucket refs have no status. This is bucket ref:

        struct bucket_ref {
            uint32_t ro;
            uint32_t rw;
            bool rw_lock;
            bool ro_lock;
        };

When the ref exists, you will try to do bucket.status which will throw an exception. The questions remains - how do tests pass if the code will fail always when any of these buckets have an existing ref object?

[mrForza]

I deleted the assertion in which we check bucket.status. The ref guarantees us that status of bucket will be ACTIVE or PINNED.

[Gerold103]

Not exactly. Ref might also exist for a bucket in READONLY or SENDING state. Or even for SENT. But in this case it shouldn't matter. We are only checking a previously created ref. If it exists, it means a bucket couldn't change its status, I suppose. It might be readonly in theory somehow, but I can't see how at the moment. @Serpentian , to our talk about storage refs vs buckets being readable/writable.

[Gerold103]

Nit: the diff would have been smaller, if you would change the if to mode ~= MAP_CALLRW_FULL, and leave else unchanged.

[mrForza]

Fixed

[Gerold103]

Unnecessary diff.

[mrForza]

Fixed

[Gerold103]

Since it was decided to reuse the old function storage_ref_make_with_buckets() with a new option force, it means this code will work on old storages too, but they will ignore the force option. You need to check for is_done result to make sure the ref is actually done. Otherwise you have the same problem as before when some storage isn't upgraded, and all its mentioned buckets were moved.

Or do we rely on total being not set on old storages, and then it fails further below? Still, at least an assertion of is_done would be a good enough safe guard.

[mrForza]

Or do we rely on total being not set on old storages, and then it fails further below?

Yes, the storage_ref_make_with_bucket on old storage can't return total amount of buckets and on router we get UNSUPPORTED error when we find out that there is no total field in the result.

Still, at least an assertion of is_done would be a good enough safe guard.

I don't think that is_done assertion can help us. When we do map_callrw on upgraded storages, we can't nicely determine which result comes from storage_ref and which comes from storage_ref_make_with_bucket because before that we unify results so that they all have a total field. And it may be that one res has total and is_done fields (storage_ref_make_with_buckets) and another has total but doesn't has is_done field (storage_ref).

We can write a small crutch)

diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index bf3da49..087ef7d 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -914,6 +914,9 @@ local function router_ref_storage_all(router, bucket_ids, timeout, rid)
                                 'full map_callrw with split args')
             goto fail
         end
+        if type(res.moved) == 'table' then
+            assert(res.is_done)
+        end
         bucket_count = bucket_count + res.total
     end
     bucket_ids = router_map_callrw_process_moved(router, results)

Or we can modify storage_ref_* functions so that they return its' identification. E.g. storage_ref will return {name = 'storage_ref', total = ...}

[Gerold103]

No need for or false. The effect will be the same anyway.

[mrForza]

Fixed