From: imeevma@tarantool.org To: v.shpilevoy@tarantool.org Cc: vdavydov.dev@gmail.com, tarantool-patches@freelists.org Subject: [PATCH v1 1/1] netbox: follow-ups Date: Sat, 22 Jun 2019 14:13:11 +0300 [thread overview] Message-ID: <95dd3d5749e69c319679dd7e6ff37c2dcc353aaa.1561200656.git.imeevma@gmail.com> (raw) Hi! Thank you for review! I combined all three review-fix patches into one follow-up patch. Also, I included a diff between the current lua/net_box.lua and the one that was before the original original patches of the issue. My answers, diff for net_box.lua and new patch below. https://github.com/tarantool/tarantool/issues/2978 https://github.com/tarantool/tarantool/tree/imeevma/gh-2978-follow-ups On 6/21/19 11:39 PM, Vladislav Shpilevoy wrote: > Hi! Thanks for the patchset! > > First of all I would like to say, that I impressed by its > simplicity, good work. I have only some minor comments except > one concerning internal netbox API and perf. > > See 3 comments below. > > 1. It would be great to see here a comment on why does all > these methods need request args saved. > > Secondly, why do you need the args for call/eval/ping/execute? > These function does not return tuples from a concrete space. > > For DML/DQL you need format ptr only, so why do you store the whole > set of parameters? You could store just one scalar value. See below > how. > > 2. You could keep these functions as is, couldn't you? The problem with current > version is that before your patch method_encoder.call would invoke C function > directly. After your patch each 'call' firstly lookups 'internal' table. This is > why me and Nick were trying to avoid calling any 'internal' functions via > accessing 'internal' table. > > 3. Problem with wrapping each time the parameters into a table is > that table wrap is usually orders of magnitude slower than variable > args pass via stack. It was benched recently. If the patch had not > been pushed, my suggestion would be to make request() method as > > perform_async_request(buffer, skip_header, method, on_push, on_push_ctx, > *request_ctx*, ...) > > This function internally would do > > request.ctx = request_ctx > > That 'ctx' would be ignored by all decoders except DML/DQL. The latter > will store format pointer here. For other requests it would be nil, and > costs nothing. 'nil' on Lua stack is stored as a special value and does > not occupy memory except stack cell. > > Motivation of my proposal is that netbox *is used* in highload, despite > we sometimes forget about it. In vshard netbox is used extremely hard. Each > new request table member costs perf and memory. For example, if we have > 300k RPS from netbox connections on one instance (it is a real number), and > args table pack costs us +1us (in fact even more), then it is not hard to > calculate, that each second we will spend nearly 3ms on packing args. > Taking into account the fact, that network delays are 'ms' too, it looks > quite expensive. > > https://github.com/tarantool/vshard/issues/186#issuecomment-500380508 > Thank you for proposal. I mostly reverted original patch and applied this solution. On 6/21/19 11:39 PM, Vladislav Shpilevoy wrote: > Thanks for the patch! > >> + >> +static int >> +lbox_tuple_format_new(struct lua_State *L) >> +{ >> + assert(CTID_STRUCT_TUPLE_FORMAT_PTR != 0); >> + if (lua_gettop(L) == 0) >> + return lbox_push_tuple_format(L, tuple_format_runtime); >> + if (lua_gettop(L) > 1 || ! lua_istable(L, 1)) >> + return luaL_error(L, "Usage: box.internal.format_new(format)"); >> + uint32_t count = lua_objlen(L, 1); >> + if (count == 0) >> + return lbox_push_tuple_format(L, tuple_format_runtime); >> + size_t size = count * sizeof(struct field_def); >> + struct region *region = &fiber()->gc; >> + size_t region_svp = region_used(region); >> + struct field_def *fields = >> + (struct field_def *)region_alloc(region, size); >> + if (fields == NULL) { >> + diag_set(OutOfMemory, size, "region_alloc", "fields"); >> + return luaT_error(L); >> + } >> + for (uint32_t i = 0; i < count; ++i) { >> + size_t len; >> + >> + fields[i] = field_def_default; >> + >> + lua_pushinteger(L, i + 1); >> + lua_gettable(L, 1); >> + >> + lua_pushstring(L, "type"); >> + lua_gettable(L, -2); >> + if (! lua_isnil(L, -1)) { >> + const char *type_name = lua_tolstring(L, -1, &len); >> + fields[i].type = field_type_by_name(type_name, len); >> + if (fields[i].type == field_type_MAX) { >> + const char *err = >> + "field %d has unknown field type"; >> + return luaL_error(L, tt_sprintf(err, i + 1)); > > Here and in all other 'return on error' the region leaks. But there is > another point. How can there be errors, if the method is internal? If I > had implemented that patch, I would panic/assert on any error here, except > region OOM. Fixed. Also, I removed tests that checked these errors. On 6/21/19 11:39 PM, Vladislav Shpilevoy wrote: > Thanks for the patch! > >> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c >> @@ -618,6 +618,13 @@ static int >> netbox_decode_select(struct lua_State *L) >> { >> uint32_t ctypeid; >> + int top = lua_gettop(L); >> + assert(top == 1 || top == 2); >> + struct tuple_format *format; >> + if (top == 2 && lua_type(L, 2) == LUA_TCDATA) >> + format = lbox_check_tuple_format(L, 2); > > How is it possible, that we do not have a format here? > it is possible since decode_call_16 calls select without format. Also, using _request() method it is possible to not receive format or receive something other than format. Examples of this can be found in box/net.box.test.lua. Diff for lua/net_box.lua: diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua index 251ad40..f7e1688 100644 --- a/src/box/lua/net_box.lua +++ b/src/box/lua/net_box.lua @@ -64,12 +64,12 @@ local function decode_data(raw_data) local response, raw_end = decode(raw_data) return response[IPROTO_DATA_KEY], raw_end end -local function decode_tuple(raw_data) - local response, raw_end = internal.decode_select(raw_data) +local function decode_tuple(raw_data, raw_data_end, format) + local response, raw_end = internal.decode_select(raw_data, format) return response[1], raw_end end -local function decode_get(raw_data) - local body, raw_end = internal.decode_select(raw_data) +local function decode_get(raw_data, raw_data_end, format) + local body, raw_end = internal.decode_select(raw_data, format) if body[2] then return nil, raw_end, box.error.MORE_THAN_ONE_TUPLE end @@ -83,6 +83,12 @@ local function decode_push(raw_data) local response, raw_end = decode(raw_data) return response[IPROTO_DATA_KEY][1], raw_end end +local function decode_call_16(raw_data) + return internal.decode_select(raw_data) +end +local function decode_select(raw_data, raw_data_end, format) + return internal.decode_select(raw_data, format) +end local method_encoder = { ping = internal.encode_ping, @@ -110,7 +116,7 @@ local method_encoder = { local method_decoder = { ping = decode_nil, - call_16 = internal.decode_select, + call_16 = decode_call_16, call_17 = decode_data, eval = decode_data, insert = decode_tuple, @@ -118,7 +124,7 @@ local method_decoder = { delete = decode_tuple, update = decode_tuple, upsert = decode_nil, - select = internal.decode_select, + select = decode_select, execute = internal.decode_execute, get = decode_get, min = decode_get, @@ -486,7 +492,7 @@ local function create_transport(host, port, user, password, callback, -- @retval not nil Future object. -- local function perform_async_request(buffer, skip_header, method, on_push, - on_push_ctx, ...) + on_push_ctx, request_ctx, ...) if state ~= 'active' and state ~= 'fetch_schema' then return nil, box.error.new({code = last_errno or E_NO_CONNECTION, reason = last_error}) @@ -499,10 +505,10 @@ local function create_transport(host, port, user, password, callback, local id = next_request_id method_encoder[method](send_buf, id, ...) next_request_id = next_id(id) - -- Request in most cases has maximum 9 members: + -- Request in most cases has maximum 10 members: -- method, buffer, skip_header, id, cond, errno, response, - -- on_push, on_push_ctx. - local request = setmetatable(table_new(0, 9), request_mt) + -- on_push, on_push_ctx and ctx. + local request = setmetatable(table_new(0, 10), request_mt) request.method = method request.buffer = buffer request.skip_header = skip_header @@ -511,6 +517,7 @@ local function create_transport(host, port, user, password, callback, requests[id] = request request.on_push = on_push request.on_push_ctx = on_push_ctx + request.ctx = request_ctx return request end @@ -520,10 +527,10 @@ local function create_transport(host, port, user, password, callback, -- @retval not nil Response object. -- local function perform_request(timeout, buffer, skip_header, method, - on_push, on_push_ctx, ...) + on_push, on_push_ctx, request_ctx, ...) local request, err = perform_async_request(buffer, skip_header, method, on_push, - on_push_ctx, ...) + on_push_ctx, request_ctx, ...) if not request then return nil, err end @@ -582,7 +589,7 @@ local function create_transport(host, port, user, password, callback, -- Decode xrow.body[DATA] to Lua objects if status == IPROTO_OK_KEY then request.response, real_end, request.errno = - method_decoder[request.method](body_rpos, body_end) + method_decoder[request.method](body_rpos, body_end, request.ctx) assert(real_end == body_end, "invalid body length") requests[id] = nil request.id = nil @@ -1064,7 +1071,7 @@ function remote_methods:wait_connected(timeout) return self._transport.wait_state('active', timeout) end -function remote_methods:_request(method, opts, ...) +function remote_methods:_request(method, opts, request_ctx, ...) local transport = self._transport local on_push, on_push_ctx, buffer, skip_header, deadline -- Extract options, set defaults, check if the request is @@ -1078,7 +1085,8 @@ function remote_methods:_request(method, opts, ...) end local res, err = transport.perform_async_request(buffer, skip_header, method, - table.insert, {}, ...) + table.insert, {}, request_ctx, + ...) if err then box.error(err) end @@ -1106,7 +1114,7 @@ function remote_methods:_request(method, opts, ...) end local res, err = transport.perform_request(timeout, buffer, skip_header, method, on_push, on_push_ctx, - ...) + request_ctx, ...) if err then box.error(err) end @@ -1133,14 +1141,14 @@ end -- @deprecated since 1.7.4 function remote_methods:call_16(func_name, ...) check_remote_arg(self, 'call') - return (self:_request('call_16', nil, tostring(func_name), {...})) + return (self:_request('call_16', nil, nil, tostring(func_name), {...})) end function remote_methods:call(func_name, args, opts) check_remote_arg(self, 'call') check_call_args(args) args = args or {} - local res = self:_request('call_17', opts, tostring(func_name), args) + local res = self:_request('call_17', opts, nil, tostring(func_name), args) if type(res) ~= 'table' or opts and opts.is_async then return res end @@ -1150,14 +1158,14 @@ end -- @deprecated since 1.7.4 function remote_methods:eval_16(code, ...) check_remote_arg(self, 'eval') - return unpack((self:_request('eval', nil, code, {...}))) + return unpack((self:_request('eval', nil, nil, code, {...}))) end function remote_methods:eval(code, args, opts) check_remote_arg(self, 'eval') check_eval_args(args) args = args or {} - local res = self:_request('eval', opts, code, args) + local res = self:_request('eval', opts, nil, code, args) if type(res) ~= 'table' or opts and opts.is_async then return res end @@ -1169,7 +1177,7 @@ function remote_methods:execute(query, parameters, sql_opts, netbox_opts) if sql_opts ~= nil then box.error(box.error.UNSUPPORTED, "execute", "options") end - return self:_request('execute', netbox_opts, query, parameters or {}, + return self:_request('execute', netbox_opts, nil, query, parameters or {}, sql_opts or {}) end @@ -1220,6 +1228,7 @@ function remote_methods:_install_schema(schema_version, spaces, indices, s.index = {} s.temporary = false s._format = format + s._format_cdata = box.internal.new_tuple_format(format) s.connection = self if #space > 5 then local opts = space[6] @@ -1314,10 +1323,12 @@ function console_methods:eval(line, timeout) end if self.protocol == 'Binary' then local loader = 'return require("console").eval(...)' - res, err = pr(timeout, nil, false, 'eval', nil, nil, loader, {line}) + res, err = pr(timeout, nil, false, 'eval', nil, nil, nil, loader, + {line}) else assert(self.protocol == 'Lua console') - res, err = pr(timeout, nil, false, 'inject', nil, nil, line..'$EOF$\n') + res, err = pr(timeout, nil, false, 'inject', nil, nil, nil, + line..'$EOF$\n') end if err then box.error(err) @@ -1336,12 +1347,12 @@ space_metatable = function(remote) function methods:insert(tuple, opts) check_space_arg(self, 'insert') - return remote:_request('insert', opts, self.id, tuple) + return remote:_request('insert', opts, self._format_cdata, self.id, tuple) end function methods:replace(tuple, opts) check_space_arg(self, 'replace') - return remote:_request('replace', opts, self.id, tuple) + return remote:_request('replace', opts, self._format_cdata, self.id, tuple) end function methods:select(key, opts) @@ -1361,7 +1372,7 @@ space_metatable = function(remote) function methods:upsert(key, oplist, opts) check_space_arg(self, 'upsert') - return nothing_or_data(remote:_request('upsert', opts, self.id, key, + return nothing_or_data(remote:_request('upsert', opts, nil, self.id, key, oplist)) end @@ -1391,8 +1402,9 @@ index_metatable = function(remote) local iterator = check_iterator_type(opts, key_is_nil) local offset = tonumber(opts and opts.offset) or 0 local limit = tonumber(opts and opts.limit) or 0xFFFFFFFF - return (remote:_request('select', opts, self.space.id, self.id, - iterator, offset, limit, key)) + return (remote:_request('select', opts, self.space._format_cdata, + self.space.id, self.id, iterator, offset, + limit, key)) end function methods:get(key, opts) @@ -1400,8 +1412,10 @@ index_metatable = function(remote) if opts and opts.buffer then error("index:get() doesn't support `buffer` argument") end - return nothing_or_data(remote:_request('get', opts, self.space.id, - self.id, box.index.EQ, 0, 2, key)) + return nothing_or_data(remote:_request('get', opts, + self.space._format_cdata, + self.space.id, self.id, + box.index.EQ, 0, 2, key)) end function methods:min(key, opts) @@ -1409,9 +1423,10 @@ index_metatable = function(remote) if opts and opts.buffer then error("index:min() doesn't support `buffer` argument") end - return nothing_or_data(remote:_request('min', opts, self.space.id, - self.id, box.index.GE, 0, 1, - key)) + return nothing_or_data(remote:_request('min', opts, + self.space._format_cdata, + self.space.id, self.id, + box.index.GE, 0, 1, key)) end function methods:max(key, opts) @@ -1419,9 +1434,10 @@ index_metatable = function(remote) if opts and opts.buffer then error("index:max() doesn't support `buffer` argument") end - return nothing_or_data(remote:_request('max', opts, self.space.id, - self.id, box.index.LE, 0, 1, - key)) + return nothing_or_data(remote:_request('max', opts, + self.space._format_cdata, + self.space.id, self.id, + box.index.LE, 0, 1, key)) end function methods:count(key, opts) @@ -1431,19 +1447,22 @@ index_metatable = function(remote) end local code = string.format('box.space.%s.index.%s:count', self.space.name, self.name) - return remote:_request('count', opts, code, { key, opts }) + return remote:_request('count', opts, nil, code, { key, opts }) end function methods:delete(key, opts) check_index_arg(self, 'delete') - return nothing_or_data(remote:_request('delete', opts, self.space.id, - self.id, key)) + return nothing_or_data(remote:_request('delete', opts, + self.space._format_cdata, + self.space.id, self.id, key)) end function methods:update(key, oplist, opts) check_index_arg(self, 'update') - return nothing_or_data(remote:_request('update', opts, self.space.id, - self.id, key, oplist)) + return nothing_or_data(remote:_request('update', opts, + self.space._format_cdata, + self.space.id, self.id, key, + oplist)) end return { __index = methods, __metatable = false } New patch: From 167c51753d973cc504595b539582794de96a66d6 Mon Sep 17 00:00:00 2001 Date: Sat, 22 Jun 2019 12:07:15 +0300 Subject: [PATCH] netbox: optimize netbox requests This patch optimizes some netbox requests that were slowed down in commit 665fc3a19c. Follow-up #2978 diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc index a835d3d..3340070 100644 --- a/src/box/lua/misc.cc +++ b/src/box/lua/misc.cc @@ -160,10 +160,10 @@ static int lbox_tuple_format_new(struct lua_State *L) { assert(CTID_STRUCT_TUPLE_FORMAT_PTR != 0); - if (lua_gettop(L) == 0) + int top = lua_gettop(L); + if (top == 0) return lbox_push_tuple_format(L, tuple_format_runtime); - if (lua_gettop(L) > 1 || ! lua_istable(L, 1)) - return luaL_error(L, "Usage: box.internal.format_new(format)"); + assert(top == 1 && lua_istable(L, 1)); uint32_t count = lua_objlen(L, 1); if (count == 0) return lbox_push_tuple_format(L, tuple_format_runtime); @@ -189,20 +189,13 @@ lbox_tuple_format_new(struct lua_State *L) if (! lua_isnil(L, -1)) { const char *type_name = lua_tolstring(L, -1, &len); fields[i].type = field_type_by_name(type_name, len); - if (fields[i].type == field_type_MAX) { - const char *err = - "field %d has unknown field type"; - return luaL_error(L, tt_sprintf(err, i + 1)); - } + assert(fields[i].type != field_type_MAX); } lua_pop(L, 1); lua_pushstring(L, "name"); lua_gettable(L, -2); - if (lua_isnil(L, -1)) { - return luaL_error(L, tt_sprintf("field %d name is not " - "specified", i + 1)); - } + assert(! lua_isnil(L, -1)); const char *name = lua_tolstring(L, -1, &len); fields[i].name = (char *)region_alloc(region, len + 1); if (fields == NULL) { diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua index 8f31712..f7e1688 100644 --- a/src/box/lua/net_box.lua +++ b/src/box/lua/net_box.lua @@ -25,6 +25,7 @@ local check_primary_index = box.internal.check_primary_index local communicate = internal.communicate local encode_auth = internal.encode_auth +local encode_select = internal.encode_select local decode_greeting = internal.decode_greeting local TIMEOUT_INFINITY = 500 * 365 * 86400 @@ -63,12 +64,12 @@ local function decode_data(raw_data) local response, raw_end = decode(raw_data) return response[IPROTO_DATA_KEY], raw_end end -local function decode_tuple(raw_data, raw_data_end, args) - local response, raw_end = internal.decode_select(raw_data, args.format) +local function decode_tuple(raw_data, raw_data_end, format) + local response, raw_end = internal.decode_select(raw_data, format) return response[1], raw_end end -local function decode_get(raw_data, raw_data_end, args) - local body, raw_end = internal.decode_select(raw_data, args.format) +local function decode_get(raw_data, raw_data_end, format) + local body, raw_end = internal.decode_select(raw_data, format) if body[2] then return nil, raw_end, box.error.MORE_THAN_ONE_TUPLE end @@ -85,67 +86,26 @@ end local function decode_call_16(raw_data) return internal.decode_select(raw_data) end -local function decode_select(raw_data, raw_data_end, args) - return internal.decode_select(raw_data, args.format) +local function decode_select(raw_data, raw_data_end, format) + return internal.decode_select(raw_data, format) end -local function encode_call(send_buf, id, args) - return internal.encode_call(send_buf, id, args.func_name, args.args) -end -local function encode_call_16(send_buf, id, args) - return internal.encode_call_16(send_buf, id, args.func_name, args.args) -end -local function encode_ping(send_buf, id, ...) - return internal.encode_ping(send_buf, id, ...) -end -local function encode_eval(send_buf, id, args) - return internal.encode_eval(send_buf, id, args.code, args.args) -end -local function encode_insert(send_buf, id, args) - return internal.encode_insert(send_buf, id, args.space_id, args.tuple) -end -local function encode_replace(send_buf, id, args) - return internal.encode_replace(send_buf, id, args.space_id, args.tuple) -end -local function encode_delete(send_buf, id, args) - return internal.encode_delete(send_buf, id, args.space_id, args.index_id, - args.key) -end -local function encode_update(send_buf, id, args) - return internal.encode_update(send_buf, id, args.space_id, args.index_id, - args.key, args.oplist) -end -local function encode_upsert(send_buf, id, args) - return internal.encode_upsert(send_buf, id, args.space_id, args.key, - args.oplist) -end -local function encode_select(send_buf, id, args) - return internal.encode_select(send_buf, id, args.space_id, args.index_id, - args.iterator, args.offset, args.limit, - args.key) -end -local function encode_execute(send_buf, id, args) - return internal.encode_execute(send_buf, id, args.query, args.parameters, - args.sql_opts) -end - - local method_encoder = { - ping = encode_ping, - call_16 = encode_call_16, - call_17 = encode_call, - eval = encode_eval, - insert = encode_insert, - replace = encode_replace, - delete = encode_delete, - update = encode_update, - upsert = encode_upsert, - select = encode_select, - execute = encode_execute, - get = encode_select, - min = encode_select, - max = encode_select, - count = encode_call, + ping = internal.encode_ping, + call_16 = internal.encode_call_16, + call_17 = internal.encode_call, + eval = internal.encode_eval, + insert = internal.encode_insert, + replace = internal.encode_replace, + delete = internal.encode_delete, + update = internal.encode_update, + upsert = internal.encode_upsert, + select = internal.encode_select, + execute = internal.encode_execute, + get = internal.encode_select, + min = internal.encode_select, + max = internal.encode_select, + count = internal.encode_call, -- inject raw data into connection, used by console and tests inject = function(buf, id, bytes) local ptr = buf:reserve(#bytes) @@ -532,7 +492,7 @@ local function create_transport(host, port, user, password, callback, -- @retval not nil Future object. -- local function perform_async_request(buffer, skip_header, method, on_push, - on_push_ctx, args) + on_push_ctx, request_ctx, ...) if state ~= 'active' and state ~= 'fetch_schema' then return nil, box.error.new({code = last_errno or E_NO_CONNECTION, reason = last_error}) @@ -543,11 +503,11 @@ local function create_transport(host, port, user, password, callback, worker_fiber:wakeup() end local id = next_request_id - method_encoder[method](send_buf, id, args) + method_encoder[method](send_buf, id, ...) next_request_id = next_id(id) -- Request in most cases has maximum 10 members: -- method, buffer, skip_header, id, cond, errno, response, - -- on_push, on_push_ctx and args. + -- on_push, on_push_ctx and ctx. local request = setmetatable(table_new(0, 10), request_mt) request.method = method request.buffer = buffer @@ -557,7 +517,7 @@ local function create_transport(host, port, user, password, callback, requests[id] = request request.on_push = on_push request.on_push_ctx = on_push_ctx - request.args = args + request.ctx = request_ctx return request end @@ -567,10 +527,10 @@ local function create_transport(host, port, user, password, callback, -- @retval not nil Response object. -- local function perform_request(timeout, buffer, skip_header, method, - on_push, on_push_ctx, args) + on_push, on_push_ctx, request_ctx, ...) local request, err = perform_async_request(buffer, skip_header, method, on_push, - on_push_ctx, args) + on_push_ctx, request_ctx, ...) if not request then return nil, err end @@ -629,8 +589,7 @@ local function create_transport(host, port, user, password, callback, -- Decode xrow.body[DATA] to Lua objects if status == IPROTO_OK_KEY then request.response, real_end, request.errno = - method_decoder[request.method](body_rpos, body_end, - request.args) + method_decoder[request.method](body_rpos, body_end, request.ctx) assert(real_end == body_end, "invalid body length") requests[id] = nil request.id = nil @@ -787,14 +746,13 @@ local function create_transport(host, port, user, password, callback, local select3_id = new_request_id() local response = {} -- fetch everything from space _vspace, 2 = ITER_ALL - internal.encode_select(send_buf, select1_id, VSPACE_ID, 0, 2, 0, - 0xFFFFFFFF, nil) + encode_select(send_buf, select1_id, VSPACE_ID, 0, 2, 0, 0xFFFFFFFF, nil) -- fetch everything from space _vindex, 2 = ITER_ALL - internal.encode_select(send_buf, select2_id, VINDEX_ID, 0, 2, 0, - 0xFFFFFFFF, nil) + encode_select(send_buf, select2_id, VINDEX_ID, 0, 2, 0, 0xFFFFFFFF, nil) -- fetch everything from space _vcollation, 2 = ITER_ALL - internal.encode_select(send_buf, select3_id, VCOLLATION_ID, 0, 2, 0, - 0xFFFFFFFF, nil) + encode_select(send_buf, select3_id, VCOLLATION_ID, 0, 2, 0, 0xFFFFFFFF, + nil) + schema_version = nil -- any schema_version will do provided that -- it is consistent across responses repeat @@ -1113,7 +1071,7 @@ function remote_methods:wait_connected(timeout) return self._transport.wait_state('active', timeout) end -function remote_methods:_request(method, opts, args) +function remote_methods:_request(method, opts, request_ctx, ...) local transport = self._transport local on_push, on_push_ctx, buffer, skip_header, deadline -- Extract options, set defaults, check if the request is @@ -1127,7 +1085,8 @@ function remote_methods:_request(method, opts, args) end local res, err = transport.perform_async_request(buffer, skip_header, method, - table.insert, {}, args) + table.insert, {}, request_ctx, + ...) if err then box.error(err) end @@ -1155,7 +1114,7 @@ function remote_methods:_request(method, opts, args) end local res, err = transport.perform_request(timeout, buffer, skip_header, method, on_push, on_push_ctx, - args) + request_ctx, ...) if err then box.error(err) end @@ -1182,16 +1141,14 @@ end -- @deprecated since 1.7.4 function remote_methods:call_16(func_name, ...) check_remote_arg(self, 'call') - local args = {func_name = tostring(func_name), args = {...}} - return (self:_request('call_16', nil, args)) + return (self:_request('call_16', nil, nil, tostring(func_name), {...})) end -function remote_methods:call(func_name, call_args, opts) +function remote_methods:call(func_name, args, opts) check_remote_arg(self, 'call') - check_call_args(call_args) - call_args = call_args or {} - local args = {func_name = tostring(func_name), args = call_args} - local res = self:_request('call_17', opts, args) + check_call_args(args) + args = args or {} + local res = self:_request('call_17', opts, nil, tostring(func_name), args) if type(res) ~= 'table' or opts and opts.is_async then return res end @@ -1201,16 +1158,14 @@ end -- @deprecated since 1.7.4 function remote_methods:eval_16(code, ...) check_remote_arg(self, 'eval') - local args = {code = code, args = {...}} - return unpack((self:_request('eval', nil, args))) + return unpack((self:_request('eval', nil, nil, code, {...}))) end -function remote_methods:eval(code, eval_args, opts) +function remote_methods:eval(code, args, opts) check_remote_arg(self, 'eval') - check_eval_args(eval_args) - eval_args = eval_args or {} - local args = {code = code, args = eval_args} - local res = self:_request('eval', opts, args) + check_eval_args(args) + args = args or {} + local res = self:_request('eval', opts, nil, code, args) if type(res) ~= 'table' or opts and opts.is_async then return res end @@ -1222,9 +1177,8 @@ function remote_methods:execute(query, parameters, sql_opts, netbox_opts) if sql_opts ~= nil then box.error(box.error.UNSUPPORTED, "execute", "options") end - local args = {query =query, parameters = parameters or {}, - sql_opts = sql_opts or {}} - return self:_request('execute', netbox_opts, args) + return self:_request('execute', netbox_opts, nil, query, parameters or {}, + sql_opts or {}) end function remote_methods:wait_state(state, timeout) @@ -1369,11 +1323,12 @@ function console_methods:eval(line, timeout) end if self.protocol == 'Binary' then local loader = 'return require("console").eval(...)' - local args = {code = loader, args = {line}} - res, err = pr(timeout, nil, false, 'eval', nil, nil, args) + res, err = pr(timeout, nil, false, 'eval', nil, nil, nil, loader, + {line}) else assert(self.protocol == 'Lua console') - res, err = pr(timeout, nil, false, 'inject', nil, nil, line..'$EOF$\n') + res, err = pr(timeout, nil, false, 'inject', nil, nil, nil, + line..'$EOF$\n') end if err then box.error(err) @@ -1392,16 +1347,12 @@ space_metatable = function(remote) function methods:insert(tuple, opts) check_space_arg(self, 'insert') - local args = {space_id = self.id, tuple = tuple, - format = self._format_cdata} - return remote:_request('insert', opts, args) + return remote:_request('insert', opts, self._format_cdata, self.id, tuple) end function methods:replace(tuple, opts) check_space_arg(self, 'replace') - local args = {space_id = self.id, tuple = tuple, - format = self._format_cdata} - return remote:_request('replace', opts, args) + return remote:_request('replace', opts, self._format_cdata, self.id, tuple) end function methods:select(key, opts) @@ -1421,8 +1372,8 @@ space_metatable = function(remote) function methods:upsert(key, oplist, opts) check_space_arg(self, 'upsert') - local args = {space_id = self.id, key = key, oplist = oplist} - return nothing_or_data(remote:_request('upsert', opts, args)) + return nothing_or_data(remote:_request('upsert', opts, nil, self.id, key, + oplist)) end function methods:get(key, opts) @@ -1451,10 +1402,9 @@ index_metatable = function(remote) local iterator = check_iterator_type(opts, key_is_nil) local offset = tonumber(opts and opts.offset) or 0 local limit = tonumber(opts and opts.limit) or 0xFFFFFFFF - local args = {space_id = self.space.id, index_id = self.id, - iterator = iterator, offset = offset, limit = limit, - key = key, format = self.space._format_cdata} - return (remote:_request('select', opts, args)) + return (remote:_request('select', opts, self.space._format_cdata, + self.space.id, self.id, iterator, offset, + limit, key)) end function methods:get(key, opts) @@ -1462,10 +1412,10 @@ index_metatable = function(remote) if opts and opts.buffer then error("index:get() doesn't support `buffer` argument") end - local args = {space_id = self.space.id, index_id = self.id, - iterator = box.index.EQ, offset = 0, limit = 2, key = key, - format = self.space._format_cdata} - return nothing_or_data(remote:_request('get', opts, args)) + return nothing_or_data(remote:_request('get', opts, + self.space._format_cdata, + self.space.id, self.id, + box.index.EQ, 0, 2, key)) end function methods:min(key, opts) @@ -1473,10 +1423,10 @@ index_metatable = function(remote) if opts and opts.buffer then error("index:min() doesn't support `buffer` argument") end - local args = {space_id = self.space.id, index_id = self.id, - iterator = box.index.GE, offset = 0, limit = 1, key = key, - format = self.space._format_cdata} - return nothing_or_data(remote:_request('min', opts, args)) + return nothing_or_data(remote:_request('min', opts, + self.space._format_cdata, + self.space.id, self.id, + box.index.GE, 0, 1, key)) end function methods:max(key, opts) @@ -1484,10 +1434,10 @@ index_metatable = function(remote) if opts and opts.buffer then error("index:max() doesn't support `buffer` argument") end - local args = {space_id = self.space.id, index_id = self.id, - iterator = box.index.LE, offset = 0, limit = 1, key = key, - format = self.space._format_cdata} - return nothing_or_data(remote:_request('max', opts, args)) + return nothing_or_data(remote:_request('max', opts, + self.space._format_cdata, + self.space.id, self.id, + box.index.LE, 0, 1, key)) end function methods:count(key, opts) @@ -1497,22 +1447,22 @@ index_metatable = function(remote) end local code = string.format('box.space.%s.index.%s:count', self.space.name, self.name) - local args = {func_name = code, args = { key, opts }} - return remote:_request('count', opts, args) + return remote:_request('count', opts, nil, code, { key, opts }) end function methods:delete(key, opts) check_index_arg(self, 'delete') - local args = {space_id = self.space.id, index_id = self.id, key = key, - format = self.space._format_cdata} - return nothing_or_data(remote:_request('delete', opts, args)) + return nothing_or_data(remote:_request('delete', opts, + self.space._format_cdata, + self.space.id, self.id, key)) end function methods:update(key, oplist, opts) check_index_arg(self, 'update') - local args = {space_id = self.space.id, index_id = self.id, key = key, - oplist = oplist, format = self.space._format_cdata} - return nothing_or_data(remote:_request('update', opts, args)) + return nothing_or_data(remote:_request('update', opts, + self.space._format_cdata, + self.space.id, self.id, key, + oplist)) end return { __index = methods, __metatable = false } diff --git a/test/box/access.result b/test/box/access.result index e3d4036..ca2531f 100644 --- a/test/box/access.result +++ b/test/box/access.result @@ -890,24 +890,15 @@ LISTEN = require('uri').parse(box.cfg.listen) c = (require 'net.box').connect(LISTEN.host, LISTEN.service) --- ... -args = {space_id=1, index_id=0, iterator=box.index.EQ, offset=0, limit=0xFFFFFFFF, key={}} ---- -... -c:_request("select", nil, args) +c:_request("select", nil, nil, 1, box.index.EQ, 0, 0, 0xFFFFFFFF, {}) --- - error: Space '1' does not exist ... -args.space_id = 65537 ---- -... -c:_request("select", nil, args) +c:_request("select", nil, nil, 65537, box.index.EQ, 0, 0, 0xFFFFFFFF, {}) --- - error: Space '65537' does not exist ... -args.space_id = 4294967295 ---- -... -c:_request("select", nil, args) +c:_request("select", nil, nil, 4294967295, box.index.EQ, 0, 0, 0xFFFFFFFF, {}) --- - error: Space '4294967295' does not exist ... diff --git a/test/box/access.test.lua b/test/box/access.test.lua index c63152b..c1ba002 100644 --- a/test/box/access.test.lua +++ b/test/box/access.test.lua @@ -343,12 +343,9 @@ box.schema.func.drop(name) -- very large space id, no crash occurs. LISTEN = require('uri').parse(box.cfg.listen) c = (require 'net.box').connect(LISTEN.host, LISTEN.service) -args = {space_id=1, index_id=0, iterator=box.index.EQ, offset=0, limit=0xFFFFFFFF, key={}} -c:_request("select", nil, args) -args.space_id = 65537 -c:_request("select", nil, args) -args.space_id = 4294967295 -c:_request("select", nil, args) +c:_request("select", nil, nil, 1, box.index.EQ, 0, 0, 0xFFFFFFFF, {}) +c:_request("select", nil, nil, 65537, box.index.EQ, 0, 0, 0xFFFFFFFF, {}) +c:_request("select", nil, nil, 4294967295, box.index.EQ, 0, 0, 0xFFFFFFFF, {}) c:close() session = box.session diff --git a/test/box/misc.result b/test/box/misc.result index 5c42e33..d21b86a 100644 --- a/test/box/misc.result +++ b/test/box/misc.result @@ -1273,66 +1273,6 @@ s:drop() ... -- -- gh-2978: Function to parse space format. --- --- Error: no field name: -format = {} ---- -... -format[1] = {} ---- -... -format[1].type = 'unsigned' ---- -... -box.internal.new_tuple_format(format) ---- -- error: field 1 name is not specified -... --- Error: duplicate field name: -format[1].name = 'aaa' ---- -... -format[2] = {} ---- -... -format[2].name = 'aaa' ---- -... -format[2].type = 'any' ---- -... -box.internal.new_tuple_format(format) ---- -- error: Space field 'aaa' is duplicate -... --- Error: wrong field type: -format[2].name = 'bbb' ---- -... -format[3] = {} ---- -... -format[3].name = 'ccc' ---- -... -format[3].type = 'something' ---- -... -box.internal.new_tuple_format(format) ---- -- error: field 3 has unknown field type -... --- Error: too many arguments: -box.internal.new_tuple_format(format, format) ---- -- error: 'Usage: box.internal.format_new(format)' -... --- Error: wrong argument: -box.internal.new_tuple_format(1) ---- -- error: 'Usage: box.internal.format_new(format)' -... --- -- In next tests we should receive cdata("struct tuple_format *"). -- We do not have a way to check cdata in Lua, but there should be -- no errors. @@ -1342,7 +1282,13 @@ tuple_format = box.internal.new_tuple_format() --- ... -- If no type that type == "any": -format[3].type = nil +format = {} +--- +... +format[1] = {} +--- +... +format[1].name = 'aaa' --- ... tuple_format = box.internal.new_tuple_format(format) @@ -1353,7 +1299,7 @@ tuple_format = box.internal.new_tuple_format(box.space._space:format()) --- ... -- Check is_nullable option fo field -format[2].is_nullable = true +format[1].is_nullable = true --- ... tuple_format = box.internal.new_tuple_format(format) diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua index 94a6450..a777777 100644 --- a/test/box/misc.test.lua +++ b/test/box/misc.test.lua @@ -353,38 +353,8 @@ lsn == expected_lsn box.cfg{too_long_threshold = too_long_threshold} s:drop() - -- -- gh-2978: Function to parse space format. --- - --- Error: no field name: -format = {} -format[1] = {} -format[1].type = 'unsigned' -box.internal.new_tuple_format(format) - --- Error: duplicate field name: -format[1].name = 'aaa' -format[2] = {} -format[2].name = 'aaa' -format[2].type = 'any' -box.internal.new_tuple_format(format) - --- Error: wrong field type: -format[2].name = 'bbb' -format[3] = {} -format[3].name = 'ccc' -format[3].type = 'something' -box.internal.new_tuple_format(format) - --- Error: too many arguments: -box.internal.new_tuple_format(format, format) - --- Error: wrong argument: -box.internal.new_tuple_format(1) - --- -- In next tests we should receive cdata("struct tuple_format *"). -- We do not have a way to check cdata in Lua, but there should be -- no errors. @@ -394,12 +364,14 @@ box.internal.new_tuple_format(1) tuple_format = box.internal.new_tuple_format() -- If no type that type == "any": -format[3].type = nil +format = {} +format[1] = {} +format[1].name = 'aaa' tuple_format = box.internal.new_tuple_format(format) -- Function space:format() without arguments returns valid format: tuple_format = box.internal.new_tuple_format(box.space._space:format()) -- Check is_nullable option fo field -format[2].is_nullable = true +format[1].is_nullable = true tuple_format = box.internal.new_tuple_format(format) diff --git a/test/box/net.box.result b/test/box/net.box.result index 1248c64..efaed11 100644 --- a/test/box/net.box.result +++ b/test/box/net.box.result @@ -25,11 +25,11 @@ test_run:cmd("setopt delimiter ';'") - true ... function x_select(cn, space_id, index_id, iterator, offset, limit, key, opts) - local args = {space_id = space_id, index_id = index_id, iterator = iterator, - offset = offset, limit = limit, key = key} - return cn:_request('select', opts, args) + local ret = cn:_request('select', opts, nil, space_id, index_id, iterator, + offset, limit, key) + return ret end -function x_fatal(cn) cn._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') end +function x_fatal(cn) cn._transport.perform_request(nil, nil, false, 'inject', nil, nil, nil, '\x80') end test_run:cmd("setopt delimiter ''"); --- ... @@ -2815,7 +2815,7 @@ c.space.test:delete{1} -- -- Break a connection to test reconnect_after. -- -_ = c._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') +_ = c._transport.perform_request(nil, nil, false, 'inject', nil, nil, nil, '\x80') --- ... c.state @@ -3456,7 +3456,7 @@ c = net:connect(box.cfg.listen, {reconnect_after = 0.01}) future = c:call('long_function', {1, 2, 3}, {is_async = true}) --- ... -_ = c._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') +_ = c._transport.perform_request(nil, nil, false, 'inject', nil, nil, nil, '\x80') --- ... while not c:is_connected() do fiber.sleep(0.01) end @@ -3689,7 +3689,7 @@ c:ping() -- new attempts to read any data - the connection is closed -- already. -- -f = fiber.create(c._transport.perform_request, nil, nil, false, 'call_17', nil, nil, 'long', {}) c._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') +f = fiber.create(c._transport.perform_request, nil, nil, false, 'call_17', nil, nil, nil, 'long', {}) c._transport.perform_request(nil, nil, false, 'inject', nil, nil, nil, '\x80') --- ... while f:status() ~= 'dead' do fiber.sleep(0.01) end @@ -3708,7 +3708,7 @@ c = net:connect(box.cfg.listen) data = msgpack.encode(18400000000000000000)..'aaaaaaa' --- ... -c._transport.perform_request(nil, nil, false, 'inject', nil, nil, data) +c._transport.perform_request(nil, nil, false, 'inject', nil, nil, nil, data) --- - null - Peer closed diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua index f222ad9..a140990 100644 --- a/test/box/net.box.test.lua +++ b/test/box/net.box.test.lua @@ -8,11 +8,11 @@ test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '") test_run:cmd("setopt delimiter ';'") function x_select(cn, space_id, index_id, iterator, offset, limit, key, opts) - local args = {space_id = space_id, index_id = index_id, iterator = iterator, - offset = offset, limit = limit, key = key} - return cn:_request('select', opts, args) + local ret = cn:_request('select', opts, nil, space_id, index_id, iterator, + offset, limit, key) + return ret end -function x_fatal(cn) cn._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') end +function x_fatal(cn) cn._transport.perform_request(nil, nil, false, 'inject', nil, nil, nil, '\x80') end test_run:cmd("setopt delimiter ''"); LISTEN = require('uri').parse(box.cfg.listen) @@ -1155,7 +1155,7 @@ c.space.test:delete{1} -- -- Break a connection to test reconnect_after. -- -_ = c._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') +_ = c._transport.perform_request(nil, nil, false, 'inject', nil, nil, nil, '\x80') c.state while not c:is_connected() do fiber.sleep(0.01) end c:ping() @@ -1388,7 +1388,7 @@ finalize_long() -- c = net:connect(box.cfg.listen, {reconnect_after = 0.01}) future = c:call('long_function', {1, 2, 3}, {is_async = true}) -_ = c._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') +_ = c._transport.perform_request(nil, nil, false, 'inject', nil, nil, nil, '\x80') while not c:is_connected() do fiber.sleep(0.01) end finalize_long() future:wait_result(100) @@ -1473,7 +1473,7 @@ c:ping() -- new attempts to read any data - the connection is closed -- already. -- -f = fiber.create(c._transport.perform_request, nil, nil, false, 'call_17', nil, nil, 'long', {}) c._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') +f = fiber.create(c._transport.perform_request, nil, nil, false, 'call_17', nil, nil, nil, 'long', {}) c._transport.perform_request(nil, nil, false, 'inject', nil, nil, nil, '\x80') while f:status() ~= 'dead' do fiber.sleep(0.01) end c:close() @@ -1483,7 +1483,7 @@ c:close() -- c = net:connect(box.cfg.listen) data = msgpack.encode(18400000000000000000)..'aaaaaaa' -c._transport.perform_request(nil, nil, false, 'inject', nil, nil, data) +c._transport.perform_request(nil, nil, false, 'inject', nil, nil, nil, data) c:close() test_run:grep_log('default', 'too big packet size in the header') ~= nil
next reply other threads:[~2019-06-22 11:13 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-22 11:13 imeevma [this message] 2019-06-22 14:21 ` [tarantool-patches] " Vladislav Shpilevoy 2019-06-25 15:10 ` Mergen Imeev 2019-06-25 16:12 ` Mergen Imeev 2019-06-25 17:57 ` [tarantool-patches] " Vladislav Shpilevoy 2019-06-27 12:31 ` [tarantool-patches] " Vladimir Davydov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=95dd3d5749e69c319679dd7e6ff37c2dcc353aaa.1561200656.git.imeevma@gmail.com \ --to=imeevma@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --cc=vdavydov.dev@gmail.com \ --subject='Re: [PATCH v1 1/1] netbox: follow-ups' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox