Feat: implement partial map-reduce #442

darthunix · 2023-11-07T10:35:17Z

Extend ref-map-reduce API with a new partial execution method:

vshard.router.partial_callrw(bucket_ids, func, args, opts, callback)

It allows users to call a function exactly once on all masters that contain buckets from the list.

Also current PR improves the connection establishment for all ref-map-reduce methods.

Gerold103

Thanks for the patch! Good stuff, but we need to work on it a bit.

README.md

test/router-luatest/router_test.lua

vshard/router/init.lua

vshard/storage/init.lua

vshard/router/init.lua

Gerold103

Thanks for the patch!

vshard/storage/init.lua

vshard/router/init.lua

test/router-luatest/router_test.lua

vshard/replicaset.lua

vshard/router/init.lua

Gerold103

Thanks for the fixes! We are on the finish line already.

vshard/storage/init.lua

test/storage-luatest/storage_1_test.lua

Gerold103 · 2024-01-10T20:30:27Z

test/storage-luatest/storage_1_test.lua

+        local bid_extra = 3001
+        local timeout = 0.1
+        local rid = 42
+        local res, err = ivshard.storage._call(
+            'storage_ref_with_lookup',
+            rid,
+            timeout,
+            {bid1, bid_extra}
+        )
+        ilt.assert_equals(err, nil)
+        ilt.assert_equals(res, {bid_extra})
+        ivshard.storage._call('storage_unref', rid)


Please, cover:

An error. And ensure that a ref wasn't made then.

More than one bucket missing. "One" is a corner case which might be working by luck.

More than one bucket missing

This one is still not covered.

Take a look at the "multiple buckets" section of the test_ref_with_lookup().

The moved result of storage_ref_with_lookup() is always empty or just one bucket, this is my point. I can't see a case when moved has more than a single bucket in it. I think this is important to test because by accident now or some time afterwards somebody might break the storage code so it would exit on the first loop iteration, and your tests would still pass.

vshard/router/init.lua

test/router-luatest/router_test.lua

Gerold103

Thanks for the fixes!

test/instances/storage.lua

vshard/router/init.lua

vshard/replicaset.lua

test/router-luatest/router_test.lua

Gerold103 · 2024-02-05T19:57:53Z

test/router-luatest/router_test.lua

+    res = g.router:exec(function(bid1, bid2)
+        local val, err, err_uuid = ivshard.router.map_part_callrw({bid2, bid1}, 'do_map', {3},
+            {timeout = iwait_timeout})
+        return {
+            val = val,
+            err = err,
+            err_uuid = err_uuid,
+        }
+    end, {bid1, bid2})
+    t.assert(res.val == nil)
+    t.assert_covers(res.err, {
+        code = box.error.PROC_LUA,
+        type = 'ClientError',
+        message = 'map_err'
+    })
+    t.assert_equals(res.err_uuid, rs2_uuid)


Please, add a check after this, that no dangling refs are left on the storages. For that you can check vshard.storage.ref.count == 0 on both instances.

I have checked handling unref error

I can't find it anywhere. Can you please point at a specific location?

I have really lost this code on rebases. Fixed

Nope, still no check for absence of references.

Gerold103

Thanks for the fixes! We still seem to be stuck with some comments. Perhaps you don't see them all. Here are the links:

test/router-luatest/router_test.lua

vshard/router/init.lua

test/storage-luatest/storage_1_test.lua

Introduce a partial ref-map-reduce API for vshard. It guarantees that in case of success the function is executed exactly once on the storages, that contain the given list of buckets. Algorithm. Router: 1. Group buckets by replicasets based on the router's cache. 2. Ref stage. For each storage: a. Async send ref id, timeout and a group of the corresponding buckets to the storage. The aim is to reference this storage and check what buckets are absent. Storage: 3. Refer session with the ref id and timeout, passed from the router. 4. Lookup for the passed buckets. If any of them were not found on the storage, return these buckets back in response to the router. Router: 5. Await and collect returned responses. If timeout has expired, set the error for this response. 6. If any of responses contains error,send unref to the refed storages and return the error to the user. 7. If the collected results contain moved buckets, search for them and update the router's cache. Decrease the timeout and goto 1. 8. Map stage. For each storage: a. Replace a bucket list with a group of buckets refed on the target storage. b. Async send a map function with modified arguments and a ref id to the storage. Storage: 9. Execute storage_map: if the ref id has expired, return error. Otherwise, ref.use -> execute -> ref.del from storage_map(). Return results. Router: 10. Reduce stage. Await results (and optionally apply a callback to each result): if timeout expired, return error to the user. Otherwise, return result.

Gerold103

Thanks for the fixes! Hm, still some of the old comments remain ignored. Lets recite them all here again (old and new ones together):

Feat: implement partial map-reduce #442 (comment) - missing a test case.
Feat: implement partial map-reduce #442 (comment) - export of an unnecessary function.
Feat: implement partial map-reduce #442 (comment) - no test for more than one moved bucket (see my latest response).
Feat: implement partial map-reduce #442 (comment) - no test for absence of dangling references (the current test will pass even if there are references).

Also have a look at CI, please. It is completely red.

Gerold103 · 2024-03-12T21:04:58Z

test/router-luatest/map_part_test.lua

+    -- Call a partial map one more time to make sure there are no references left.
+    res = g.router:exec(function(bid1, bid2)
+        local val, err, err_uuid = ivshard.router.map_part_callrw(
+            {bid1, bid2}, 'do_map', {42}, {timeout = iwait_timeout})


How does calling map_part_callrw() second time ensures that there are no references left? It will succeed even if there are references anyway.

Gerold103 · 2024-03-12T21:06:02Z

test/router-luatest/map_part_test.lua

+    -- Check that there is no dangling references after the error.
+    init = map_part_init()
+    res = g.router:exec(function(bid1, bid2)
+        local val, err, err_uuid = ivshard.router.map_part_callrw({bid1, bid2}, 'do_map', {3},
+            {timeout = iwait_timeout})


Ditto. The only 2 ways to check if there are no references left is to 1) try to send a bucket, 2) manually check vshard.storage.ref.count == 0. The second is the simplest way.

Gerold103 · 2024-03-12T21:09:50Z

test/instances/storage.lua

@@ -13,6 +13,7 @@ _G.ivconst = require('vshard.consts')
 _G.ivutil = require('vshard.util')
 _G.iverror = require('vshard.error')
 _G.ivtest = require('test.luatest_helpers.vtest')
+_G.itable_new = require('table.new')


You don't need it, table.new() is already available in the global namespace.

Gerold103 · 2024-10-10T17:18:42Z

Finished in #491.

darthunix marked this pull request as ready for review November 7, 2023 14:58

Gerold103 self-requested a review November 9, 2023 20:18

Gerold103 requested changes Nov 13, 2023

View reviewed changes

darthunix marked this pull request as draft November 25, 2023 07:35

darthunix force-pushed the multicall branch 4 times, most recently from 4950115 to 0995a72 Compare November 28, 2023 06:19

darthunix marked this pull request as ready for review November 28, 2023 06:19

darthunix requested a review from Gerold103 November 28, 2023 06:19

darthunix force-pushed the multicall branch from 0995a72 to 7b1d863 Compare November 29, 2023 05:07

darthunix marked this pull request as draft November 29, 2023 06:14

darthunix marked this pull request as ready for review November 29, 2023 06:40

darthunix force-pushed the multicall branch from 7b1d863 to 82d9669 Compare November 29, 2023 09:33

darthunix marked this pull request as draft November 29, 2023 10:08

darthunix marked this pull request as ready for review November 30, 2023 09:32

darthunix changed the title ~~Feat: implement multicall~~ Feat: implement partial map-reduce Nov 30, 2023

darthunix commented Dec 1, 2023

View reviewed changes

vshard/router/init.lua Outdated Show resolved Hide resolved

darthunix force-pushed the multicall branch from 776757a to ddc6937 Compare December 5, 2023 07:17

Gerold103 requested changes Dec 19, 2023

View reviewed changes

darthunix force-pushed the multicall branch 3 times, most recently from dccd824 to 519dadc Compare December 26, 2023 12:39

darthunix requested a review from Gerold103 December 28, 2023 03:54

sergos assigned Gerold103 Dec 29, 2023

Gerold103 requested changes Jan 10, 2024

View reviewed changes

darthunix force-pushed the multicall branch 2 times, most recently from 5951459 to b5abf70 Compare January 29, 2024 10:15

darthunix requested a review from Gerold103 January 29, 2024 10:17

Gerold103 requested changes Feb 5, 2024

View reviewed changes

darthunix force-pushed the multicall branch 2 times, most recently from 995ba20 to b80d17c Compare February 14, 2024 09:08

darthunix requested a review from Gerold103 February 14, 2024 09:10

Gerold103 requested changes Feb 19, 2024

View reviewed changes

test/router-luatest/router_test.lua Outdated Show resolved Hide resolved

test/router-luatest/router_test.lua Outdated Show resolved Hide resolved

vshard/router/init.lua Outdated Show resolved Hide resolved

test/storage-luatest/storage_1_test.lua Outdated Show resolved Hide resolved

darthunix force-pushed the multicall branch 2 times, most recently from d261e12 to d0074ff Compare March 6, 2024 07:02

storage: implement partial map-reduce API

ac4c5b1

darthunix force-pushed the multicall branch from d0074ff to 9375bac Compare March 6, 2024 08:10

darthunix added 2 commits March 6, 2024 17:13

router: improve master connection parallelism in map-reduce

d4c4c4a

darthunix force-pushed the multicall branch from 9375bac to d4c4c4a Compare March 6, 2024 10:13

darthunix requested a review from Gerold103 March 6, 2024 10:14

Gerold103 requested changes Mar 12, 2024

View reviewed changes

Gerold103 mentioned this pull request Sep 23, 2024

Partial Map-Reduce #491

Merged

Gerold103 closed this Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: implement partial map-reduce #442

Feat: implement partial map-reduce #442

darthunix commented Nov 7, 2023 •

edited

Loading

Gerold103 left a comment

Gerold103 left a comment

Gerold103 left a comment

Gerold103 Jan 10, 2024

Gerold103 Feb 5, 2024

darthunix Mar 6, 2024

Gerold103 Mar 12, 2024

Gerold103 left a comment

Gerold103 Feb 5, 2024

darthunix Feb 14, 2024

Gerold103 Feb 19, 2024

darthunix Mar 6, 2024

Gerold103 Mar 12, 2024

Gerold103 left a comment

Gerold103 left a comment •

edited

Loading

Gerold103 Mar 12, 2024

Gerold103 Mar 12, 2024

Gerold103 Mar 12, 2024

Gerold103 commented Oct 10, 2024

Feat: implement partial map-reduce #442

Feat: implement partial map-reduce #442

Conversation

darthunix commented Nov 7, 2023 • edited Loading

Gerold103 left a comment

Choose a reason for hiding this comment

Gerold103 left a comment

Choose a reason for hiding this comment

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

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

Gerold103 left a comment

Choose a reason for hiding this comment

Gerold103 left a comment • edited Loading

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

Gerold103 commented Oct 10, 2024

darthunix commented Nov 7, 2023 •

edited

Loading

Gerold103 left a comment •

edited

Loading