review fixes

tarantool · Sep 18, 2024 · 12398b4 · 12398b4
1 parent e48b75d
commit 12398b4
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 76 deletions.
diff --git a/test/storage-luatest/storage_1_test.lua b/test/storage-luatest/storage_1_test.lua
@@ -228,92 +228,80 @@ end
 
 test_group.test_ref_with_lookup = function(g)
     g.replica_1_a:exec(function()
+        local lref = require('vshard.storage.ref')
         local res, err
-        local timeout = 0.1
         local rid = 42
         local bids = _G.get_n_buckets(2)
-        local bid_extra = 3001
+        local bucket_count = ivshard.storage.internal.total_bucket_count
 
-        -- Check for a single bucket.
+        -- No buckets.
         res, err = ivshard.storage._call(
-            'storage_ref_with_lookup',
-            rid,
-            timeout,
-            {bids[1]}
-        )
+            'storage_ref_with_lookup', rid, iwait_timeout, {})
+        ilt.assert_equals(err, nil)
+        ilt.assert_equals(res, {moved = {}})
+        ilt.assert_equals(lref.count, 0)
+
+        -- Check for a single ok bucket.
+        res, err = ivshard.storage._call(
+            'storage_ref_with_lookup', rid, iwait_timeout, {bids[1]})
         ilt.assert_equals(err, nil)
         ilt.assert_equals(res, {rid = rid, moved = {}})
+        ilt.assert_equals(lref.count, 1)
         res, err = ivshard.storage._call('storage_unref', rid)
         ilt.assert_equals(err, nil)
+        ilt.assert_equals(lref.count, 0)
 
-        -- Check for multiple buckets.
+        -- Check for multiple ok buckets.
         res, err = ivshard.storage._call(
-            'storage_ref_with_lookup',
-            rid,
-            timeout,
-            {bids[1], bids[2]}
-        )
+            'storage_ref_with_lookup', rid, iwait_timeout, {bids[1], bids[2]})
         ilt.assert_equals(err, nil)
         ilt.assert_equals(res, {rid = rid, moved = {}})
         res, err = ivshard.storage._call('storage_unref', rid)
         ilt.assert_equals(err, nil)
 
         -- Check for double referencing.
         res, err = ivshard.storage._call(
-            'storage_ref_with_lookup',
-            rid,
-            timeout,
-            {bids[1], bids[1]}
-        )
+            'storage_ref_with_lookup', rid, iwait_timeout, {bids[1], bids[1]})
         ilt.assert_equals(err, nil)
         ilt.assert_equals(res, {rid = rid, moved = {}})
+        ilt.assert_equals(lref.count, 1)
         res, err = ivshard.storage._call('storage_unref', rid)
         ilt.assert_equals(err, nil)
-        res, err = ivshard.storage._call('storage_unref', rid)
-        t.assert_str_contains(err.message, 'Can not delete a storage ref: no ref')
+        ilt.assert_equals(lref.count, 0)
 
-        -- Check for an absent bucket.
+        -- Bucket mix.
         res, err = ivshard.storage._call(
-            'storage_ref_with_lookup',
-            rid,
-            timeout,
-            {bids[1], bids[2], bid_extra}
-        )
+            'storage_ref_with_lookup', rid, iwait_timeout,
+            {bucket_count + 1, bids[1], bucket_count + 2, bids[2],
+             bucket_count + 3})
         ilt.assert_equals(err, nil)
-        ilt.assert_equals(res, {rid = rid, moved = {bid_extra}})
+        ilt.assert_equals(res, {
+            rid = rid,
+            moved = {bucket_count + 1, bucket_count + 2, bucket_count + 3}
+        })
         res, err = ivshard.storage._call('storage_unref', rid)
         ilt.assert_equals(err, nil)
 
-        -- Check that we do not create a reference if there are no buckets.
+        -- No ref when all buckets are missing.
         res, err = ivshard.storage._call(
             'storage_ref_with_lookup',
             rid,
-            timeout,
-            {bid_extra}
+            iwait_timeout,
+            {bucket_count + 1, bucket_count + 2}
         )
         ilt.assert_equals(err, nil)
-        ilt.assert_equals(res, {rid = nil, moved = {bid_extra}})
-        res, err = vshard.storage._call('storage_unref', rid)
-        t.assert_str_contains(err.message, 'Can not delete a storage ref: no ref')
-        ilt.assert_equals(res, nil)
+        ilt.assert_equals(res, {moved = {bucket_count + 1, bucket_count + 2}})
+        ilt.assert_equals(lref.count, 0)
 
-        -- Check for a timeout.
-        -- Emulate a case when all buckets are not writable.
-        local func = ivshard.storage.internal.bucket_are_all_rw
-        ivshard.storage.internal.bucket_are_all_rw = function() return false end
+        -- Timeout when some buckets aren't writable. Doesn't have to be the
+        -- same buckets as for moving.
+        box.space._bucket:update({bids[1]}, {{'=', 'status', ivconst.BUCKET.SENDING}})
         res, err = ivshard.storage._call(
-            'storage_ref_with_lookup',
-            rid,
-            timeout,
-            {bids[1]}
-        )
-        ivshard.storage.internal.bucket_are_all_rw = func
+            'storage_ref_with_lookup', rid, 0.01, {bids[2]})
+        box.space._bucket:update({bids[1]}, {{'=', 'status', ivconst.BUCKET.ACTIVE}})
         t.assert_str_contains(err.message, 'Timeout exceeded')
         ilt.assert_equals(res, nil)
-        -- Check that the reference was not created.
-        res, err = ivshard.storage._call('storage_unref', rid)
-        t.assert_str_contains(err.message, 'Can not delete a storage ref: no ref')
-        ilt.assert_equals(res, nil)
+        ilt.assert_equals(lref.count, 0)
     end)
 end
 
@@ -325,15 +313,13 @@ test_group.test_absent_buckets = function(g)
         )
         ilt.assert_equals(err, nil)
         ilt.assert_equals(res, {moved = {}})
-    end)
 
-    g.replica_1_a:exec(function()
-        local bid_extra = 3001
-        local res, err = ivshard.storage._call(
+        local bucket_count = ivshard.storage.internal.total_bucket_count
+        res, err = ivshard.storage._call(
             'storage_absent_buckets',
-            {_G.get_first_bucket(), bid_extra}
+            {_G.get_first_bucket(), bucket_count + 1, bucket_count + 2}
         )
         ilt.assert_equals(err, nil)
-        ilt.assert_equals(res, {moved = {bid_extra}})
+        ilt.assert_equals(res, {moved = {bucket_count + 1, bucket_count + 2}})
     end)
 end
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
@@ -3216,28 +3216,10 @@ end
 local function storage_ref_with_lookup(rid, timeout, bucket_ids)
     local moved = storage_absent_buckets(bucket_ids).moved
     if #moved == #bucket_ids then
-        -- Take an advantage that moved buckets are returned in the same
-        -- order as in the input list.
-        local do_match = true
-        local next_moved = next(moved)
-        local next_passed = next(bucket_ids)
-        ::continue::
-        if next_moved then
-            if next_moved == next_passed then
-                next_moved = next(moved, next_moved)
-                next_passed = next(bucket_ids, next_passed)
-                goto continue
-            else
-                do_match = false
-            end
-        end
-        if do_match then
-            -- If all the passed buckets are absent, there is no need
-            -- to create a ref.
-            return {rid = nil, moved = moved}
-        end
+        -- If all the passed buckets are absent, there is no need to create a
+        -- ref.
+        return {rid = nil, moved = moved}
     end
-
     local ok, err = storage_ref(rid, timeout)
     if not ok then
         return nil, err