Skip to content

Commit

Permalink
api: forbid using space id in len
Browse files Browse the repository at this point in the history
Using space id instead of a space name in sharded systems may be
dangerous until one sets all space id manually. This is a breaking
change. The feature was deprecated since 0.14.0.

Closes #255
  • Loading branch information
DifferentialOrange committed Jan 30, 2023
1 parent 5717e87 commit 506f697
Show file tree
Hide file tree
Showing 6 changed files with 3 additions and 107 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
replicaset and for the master connection (#95).

### Changed
* **Breaking**: forbid using space id in `crud.len` (#255).

### Fixed
* Added validation of the master presence in replicaset and the
Expand Down
13 changes: 1 addition & 12 deletions crud/len.lua
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
local checks = require('checks')
local errors = require('errors')
local log = require('log')

local utils = require('crud.common.utils')
local dev_checks = require('crud.common.dev_checks')
Expand Down Expand Up @@ -42,23 +41,13 @@ end
-- @treturn[2] table Error description
--
function len.call(space_name, opts)
checks('string|number', {
checks('string', {
timeout = '?number',
vshard_router = '?string|table',
})

opts = opts or {}

if type(space_name) == 'number' then
log.warn('Using space id in crud.len is deprecated and will be removed in future releases.' ..
'Please, use space name instead.')

if opts.vshard_router ~= nil then
log.warn('Using space id in crud.len and custom vshard_router is not supported by statistics.' ..
'Space labels may be inconsistent.')
end
end

local vshard_router, err = utils.get_vshard_router_instance(opts.vshard_router)
if err ~= nil then
return nil, LenError:new(err)
Expand Down
46 changes: 1 addition & 45 deletions crud/stats/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,9 @@ local checks = require('checks')
local errors = require('errors')
local fiber = require('fiber')
local fun = require('fun')
local log = require('log')
local vshard = require('vshard')

local dev_checks = require('crud.common.dev_checks')
local stash = require('crud.common.stash')
local utils = require('crud.common.utils')
local op_module = require('crud.stats.operation')

local StatsError = errors.new_class('StatsError', {capture_stack = false})
Expand Down Expand Up @@ -249,34 +246,6 @@ function stats.get(space_name)
return internal:get_registry().get(space_name)
end

local function resolve_space_name(space_id)
-- Resolving name with custom vshard router is not supported.
local vshard_router = vshard.router.static

if vshard_router == nil then
log.warn('Failed to resolve space name for stats: default vshard router not found')
return nil
end

local replicasets = vshard_router:routeall()
if next(replicasets) == nil then
log.warn('Failed to resolve space name for stats: no replicasets found with default router')
return nil
end

local space, err = utils.get_space(space_id, vshard_router)
if err ~= nil then
log.warn("An error occurred during getting space: %s", err)
return nil
end
if space == nil then
log.warn('Failed to resolve space name for stats: no space found for id %d with default router', space_id)
return nil
end

return space.name
end

-- Hack to set __gc for a table in Lua 5.1
-- See https://stackoverflow.com/questions/27426704/lua-5-1-workaround-for-gc-metamethod-for-tables
-- or https://habr.com/ru/post/346892/
Expand Down Expand Up @@ -351,26 +320,13 @@ local function wrap_pairs_gen(build_latency, space_name, op, gen, param, state)
end

local function wrap_tail(space_name, op, pairs, start_time, call_status, ...)
dev_checks('string|number', 'string', 'boolean', 'number', 'boolean')
dev_checks('string', 'string', 'boolean', 'number', 'boolean')

local finish_time = clock.monotonic()
local latency = finish_time - start_time

local registry = internal:get_registry()

-- If space id is provided instead of name, try to resolve name.
-- If resolve have failed, use id as string to observe space.
-- If using space id will be deprecated, remove this code as well,
-- see https://github.com/tarantool/crud/issues/255
if type(space_name) ~= 'string' then
local name = resolve_space_name(space_name)
if name ~= nil then
space_name = name
else
space_name = tostring(space_name)
end
end

if call_status == false then
registry.observe(latency, space_name, op, 'error')
error((...), 2)
Expand Down
1 change: 0 additions & 1 deletion test/entrypoint/srv_vshard_custom.lua
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ package.preload['customers-storage'] = function()
},
if_not_exists = true,
engine = engine,
id = 542,
})

customers_space:create_index('pk', {
Expand Down
20 changes: 0 additions & 20 deletions test/integration/stats_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@ local group_metrics = t.group('stats_metrics_integration', {

local helpers = require('test.helper')

local space_id = 542
local space_name = 'customers'
local non_existing_space_id = 100500
local non_existing_space_name = 'non_existing_space'
local new_space_name = 'newspace'

Expand Down Expand Up @@ -752,24 +750,6 @@ for name, case in pairs(select_cases) do
end


pgroup.test_resolve_name_from_id = function(g)
local op = 'len'
g.router:call('crud.len', { space_id })

local stats = get_stats(g, space_name)
t.assert_not_equals(stats[op], nil, "Statistics is filled by name")
end


pgroup.test_resolve_nonexisting_space_from_id = function(g)
local op = 'len'
g.router:call('crud.len', { non_existing_space_id })

local stats = get_stats(g, tostring(non_existing_space_id))
t.assert_not_equals(stats[op], nil, "Statistics is filled by id as string")
end


pgroup.before_test(
'test_role_reload_do_not_reset_observations',
generate_stats)
Expand Down
29 changes: 0 additions & 29 deletions test/integration/vshard_custom_test.lua
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
local fio = require('fio')

local t = require('luatest')
local luatest_capture = require('luatest.capture')

local helpers = require('test.helper')

Expand Down Expand Up @@ -1630,31 +1629,3 @@ pgroup.test_call_upsert_object_many_wrong_option = function(g)
t.assert_str_contains(errs[1].err,
"Invalid opts.vshard_router table value, a vshard router instance has been expected")
end

pgroup.before_test('test_call_len_by_space_id_with_stats', function(g)
g.router:eval('crud.cfg{stats = true}')
end)

pgroup.test_call_len_by_space_id_with_stats = function(g)
local capture = luatest_capture:new()
capture:enable()

local result, err = g:call_router_opts2('len', 542, {vshard_router = 'customers'})
t.assert_equals(err, nil)
t.assert_equals(result, 2)

local captured = helpers.fflush_main_server_stdout(g.cluster, capture)
capture:disable()

t.assert_str_contains(captured,
"Using space id in crud.len and custom vshard_router is not supported by statistics.")

local result, err = g.router:call('crud.stats')
t.assert_equals(err, nil)
t.assert_type(result.spaces["542"], 'table')
t.assert_equals(result.spaces["542"]["len"]["ok"]["count"], 1)
end

pgroup.after_test('test_call_len_by_space_id_with_stats', function(g)
g.router:eval('crud.cfg{stats = false}')
end)

0 comments on commit 506f697

Please sign in to comment.