Skip to content

Commit

Permalink
crud: fix passing errors from storages for merger
Browse files Browse the repository at this point in the history
Before this patch, returning any errors from storages for merger
operations (`crud.select`, `crud.pairs`, `readview:select`,
`readview:pairs`) had resulted in non-informative
"Invalid merge source 0x556bb6885a00" errors. This patch fixes the issue
by replacing error return with error throw.

Part of #373
  • Loading branch information
DifferentialOrange committed Mar 12, 2024
1 parent b44caab commit 074e78d
Show file tree
Hide file tree
Showing 7 changed files with 147 additions and 12 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
non-iterating indexes (#373).
* Precision loss for decimal conditions in case of non-indexed fields or
non-iterating indexes (#373).
* Passing errors from storages for merger operations (`crud.select`,
`crud.pairs`, `readview:select`, `readview:pairs`) (#423).

## [1.4.3] - 05-02-24

Expand Down
8 changes: 4 additions & 4 deletions crud/select.lua
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,12 @@ local function select_on_storage(space_name, index_id, conditions, opts)

local space = box.space[space_name]
if space == nil then
return cursor, SelectError:new("Space %q doesn't exist", space_name)
SelectError:assert(false, "Space %q doesn't exist", space_name)
end

local index = space.index[index_id]
if index == nil then
return cursor, SelectError:new("Index with ID %s doesn't exist", index_id)
SelectError:assert(false, "Index with ID %s doesn't exist", index_id)
end

local _, err = sharding.check_sharding_hash(space_name,
Expand All @@ -83,7 +83,7 @@ local function select_on_storage(space_name, index_id, conditions, opts)
scan_condition_num = opts.scan_condition_num,
})
if err ~= nil then
return cursor, SelectError:new("Failed to generate tuples filter: %s", err)
return SelectError:assert(false, "Failed to generate tuples filter: %s", err)
end

-- execute select
Expand All @@ -95,7 +95,7 @@ local function select_on_storage(space_name, index_id, conditions, opts)
yield_every = opts.yield_every,
})
if err ~= nil then
return cursor, SelectError:new("Failed to execute select: %s", err)
return SelectError:assert(false, "Failed to execute select: %s", err)
end

if resp.tuples_fetched < opts.limit or opts.limit == 0 then
Expand Down
21 changes: 19 additions & 2 deletions test/integration/pairs_readview_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -895,11 +895,14 @@ local function read_impl(cg, space, conditions, opts)
local status, resp = pcall(function()
return rv:pairs(space, conditions, opts):totable()
end)
t.assert(status, resp)

rv:close()

return resp, nil
if status then
return resp, nil
else
return nil, resp
end
end, {space, conditions, opts})
end

Expand All @@ -919,3 +922,17 @@ for case_name_template, case in pairs(gh_373_types_cases) do
case(g, read_impl)
end
end

pgroup.before_test(
'test_pairs_merger_process_storage_error',
read_scenario.before_merger_process_storage_error
)

pgroup.test_pairs_merger_process_storage_error = function(g)
read_scenario.merger_process_storage_error(g, read_impl)
end

pgroup.after_test(
'test_pairs_merger_process_storage_error',
read_scenario.after_merger_process_storage_error
)
21 changes: 19 additions & 2 deletions test/integration/pairs_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -905,9 +905,12 @@ local function read_impl(cg, space, conditions, opts)
local status, resp = pcall(function()
return crud.pairs(space, conditions, opts):totable()
end)
t.assert(status, resp)

return resp, nil
if status then
return resp, nil
else
return nil, resp
end
end, {space, conditions, opts})
end

Expand All @@ -927,3 +930,17 @@ for case_name_template, case in pairs(gh_373_types_cases) do
case(g, read_impl)
end
end

pgroup.before_test(
'test_pairs_merger_process_storage_error',
read_scenario.before_merger_process_storage_error
)

pgroup.test_pairs_merger_process_storage_error = function(g)
read_scenario.merger_process_storage_error(g, read_impl)
end

pgroup.after_test(
'test_pairs_merger_process_storage_error',
read_scenario.after_merger_process_storage_error
)
66 changes: 65 additions & 1 deletion test/integration/read_scenario.lua
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,12 @@ local function gh_418_read_with_secondary_noneq_index_condition(cg, read)
-- iterator had erroneously expected tuples to be sorted by `last_login`
-- index while iterating on `city` index. Before the issue had beed fixed,
-- user had received only one record instead of two.
local result = read(cg,
local result, err = read(cg,
'logins',
{{'=', 'city', 'Tatsumi Port Island'}, {'<=', 'last_login', 42}},
{bucket_id = PINNED_BUCKET_NO}
)
t.assert_equals(err, nil)

if type(result) == 'number' then -- crud.count
t.assert_equals(result, 2)
Expand Down Expand Up @@ -555,8 +556,71 @@ for space_kind, space_option in pairs(datetime_condition_space_options) do
end


local function before_merger_process_storage_error(cg)
helpers.call_on_storages(cg.cluster, function(server)
server:exec(function()
local space
if box.info.ro == false then
space = box.schema.space.create('speedy_gonzales', {if_not_exists = true})

space:format({
{name = 'id', type = 'unsigned'},
{name = 'bucket_id', type = 'unsigned'},
})

space:create_index('pk', {
parts = {'id'},
if_not_exists = true,
})

space:create_index('bucket_id', {
parts = {'bucket_id'},
unique = false,
if_not_exists = true,
})
end

local real_select_impl = rawget(_G, '_crud').select_on_storage
rawset(_G, '_real_select_impl', real_select_impl)

-- Drop right before select to cause storage-side error.
-- Work guaranteed only with mode = 'write'.
local function erroneous_select_impl(...)
if box.info.ro == false then
space:drop()
end

return real_select_impl(...)
end

rawget(_G, '_crud').select_on_storage = erroneous_select_impl
end)
end)
end

local function merger_process_storage_error(cg, read)
local _, err = read(cg, 'speedy_gonzales', {{'==', 'id', 1}})
t.assert_not_equals(err, nil)

local err_msg = err.err or tostring(err)
t.assert_str_contains(err_msg, "Space \"speedy_gonzales\" doesn't exist")
end

local function after_merger_process_storage_error(cg)
helpers.call_on_storages(cg.cluster, function(server)
server:exec(function()
local real_select_impl = rawget(_G, '_real_select_impl')

rawget(_G, '_crud').select_on_storage = real_select_impl
end)
end)
end

return {
gh_418_read_with_secondary_noneq_index_condition = gh_418_read_with_secondary_noneq_index_condition,
gh_373_read_with_decimal_condition_cases = gh_373_read_with_decimal_condition_cases,
gh_373_read_with_datetime_condition_cases = gh_373_read_with_datetime_condition_cases,
before_merger_process_storage_error = before_merger_process_storage_error,
merger_process_storage_error = merger_process_storage_error,
after_merger_process_storage_error = after_merger_process_storage_error,
}
20 changes: 19 additions & 1 deletion test/integration/select_readview_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2498,7 +2498,11 @@ local function read_impl(cg, space, conditions, opts)

rv:close()

return crud.unflatten_rows(resp.rows, resp.metadata)
if err ~= nil then
return nil, err
end

return crud.unflatten_rows(resp.rows, resp.metadata), nil
end, {space, conditions, opts})
end

Expand All @@ -2518,3 +2522,17 @@ for case_name_template, case in pairs(gh_373_types_cases) do
case(g, read_impl)
end
end

pgroup.before_test(
'test_select_merger_process_storage_error',
read_scenario.before_merger_process_storage_error
)

pgroup.test_select_merger_process_storage_error = function(g)
read_scenario.merger_process_storage_error(g, read_impl)
end

pgroup.after_test(
'test_select_merger_process_storage_error',
read_scenario.after_merger_process_storage_error
)
21 changes: 19 additions & 2 deletions test/integration/select_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2274,9 +2274,12 @@ local function read_impl(cg, space, conditions, opts)
opts.mode = 'write'

local resp, err = cg.cluster.main_server:call('crud.select', {space, conditions, opts})
t.assert_equals(err, nil)

return crud.unflatten_rows(resp.rows, resp.metadata)
if err ~= nil then
return nil, err
end

return crud.unflatten_rows(resp.rows, resp.metadata), nil
end

pgroup.test_gh_418_select_with_secondary_noneq_index_condition = function(g)
Expand All @@ -2295,3 +2298,17 @@ for case_name_template, case in pairs(gh_373_types_cases) do
case(g, read_impl)
end
end

pgroup.before_test(
'test_select_merger_process_storage_error',
read_scenario.before_merger_process_storage_error
)

pgroup.test_select_merger_process_storage_error = function(g)
read_scenario.merger_process_storage_error(g, read_impl)
end

pgroup.after_test(
'test_select_merger_process_storage_error',
read_scenario.after_merger_process_storage_error
)

0 comments on commit 074e78d

Please sign in to comment.