Skip to content

api: forbid using space id in len #338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)