From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id E0EC0241F0 for ; Tue, 8 May 2018 13:24:37 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id AvVTmv5FVmuT for ; Tue, 8 May 2018 13:24:37 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 710522087C for ; Tue, 8 May 2018 13:24:36 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 4/8] netbox: extend codec with 'decode' methods References: <5196d19705a653bfbf600cf787e05f4f75df99f6.1523903144.git.v.shpilevoy@tarantool.org> <20180508154934.GD4867@atlas> From: Vladislav Shpilevoy Message-ID: <57cef482-d13b-d0d8-7f3c-a58e601e796c@tarantool.org> Date: Tue, 8 May 2018 20:24:33 +0300 MIME-Version: 1.0 In-Reply-To: <20180508154934.GD4867@atlas> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Konstantin Osipov Hello. Thanks for review. On 08/05/2018 18:49, Konstantin Osipov wrote: > * Vladislav Shpilevoy [18/04/16 21:44]: >> Netbox has a table 'method_codec' that is used to encode a >> request by a method name. But a response is decoded out of codec. >> It leads to >> 1) decoding into Lua tables before decoding into tuples where >> needed - it is double decoding and produces a lot of garbage; >> 2) each method contains hacks like one_tuple(), or single tuple >> check. >> >> These things can not be fixed with no real codec instead of >> encoder only. >> >> Also global table with decoders is needed for #3107, where >> a request could be sent async with no fiber blocking. An async >> response when received already does not have a call context - it >> has only method name. >> >> Needed for #3107 >> --- >> src/box/lua/net_box.lua | 116 +++++++++++++++++++++++++++++------------------- >> test/box/net.box.result | 14 ++++++ >> test/box/sql.result | 2 + >> 3 files changed, 87 insertions(+), 45 deletions(-) >> >> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua >> index 4ed2b375d..3868cdf1c 100644 >> --- a/src/box/lua/net_box.lua >> +++ b/src/box/lua/net_box.lua >> @@ -50,7 +50,34 @@ local E_PROC_LUA = box.error.PROC_LUA >> >> -- utility tables >> local is_final_state = {closed = 1, error = 1} >> -local method_codec = { >> + >> +local function decode_nil(...) end >> +local function decode_nothing(...) return ... end > > decode_nothing -> decode_nop You reviewed the old patch. Is is fixed already on Vova request. > >> +local function decode_one_tuple(response) >> + if response[1] then >> + return box.tuple.new(response[1]) >> + end >> +end > > decode_one_tuple -> decode_tuple Ok. @@ -53,7 +53,7 @@ local is_final_state = {closed = 1, error = 1} local function decode_nil(...) end local function decode_nop(...) return ... end -local function decode_one_tuple(response) +local function decode_tuple(response) if response[1] then return box.tuple.new(response[1]) end @@ -105,10 +105,10 @@ local method_decoder = { call_16 = decode_select, call_17 = decode_nop, eval = decode_nop, - insert = decode_one_tuple, - replace = decode_one_tuple, - delete = decode_one_tuple, - update = decode_one_tuple, + insert = decode_tuple, + replace = decode_tuple, + delete = decode_tuple, + update = decode_tuple, upsert = decode_nil, select = decode_select, get = decode_unique_tuple, > > Why do you need the if () guard? Body can be empty - in such a case there is no sense to allocate cdata. > >> +local function decode_single_tuple(response) >> + if response[2] then >> + return nil, box.error.MORE_THAN_ONE_TUPLE >> + end >> + if response[1] then >> + return box.tuple.new(response[1]) >> + end >> +end > > decode_single_tuple -> decode_get() Ok. @@ -58,7 +58,7 @@ local function decode_tuple(response) return box.tuple.new(response[1]) end end -local function decode_unique_tuple(response) +local function decode_get(response) if response[2] then return nil, box.error.MORE_THAN_ONE_TUPLE end @@ -111,9 +111,9 @@ local method_decoder = { update = decode_tuple, upsert = decode_nil, select = decode_select, - get = decode_unique_tuple, - min = decode_unique_tuple, - max = decode_unique_tuple, + get = decode_get, + min = decode_get, + max = decode_get, count = decode_count, inject = decode_nop, > >> +local function decode_select(response) >> + setmetatable(response, sequence_mt) >> + for i, v in pairs(response) do >> + response[i] = box.tuple.new(v) >> + end >> + return response >> +end > > Why are you using pairs() rather than ipairs()? Pairs is faster. https://stackoverflow.com/questions/8955085/should-i-use-ipairs-or-a-for-loop > > How does this avoid double decode as you claim in changeset > comment? In no way. I did not said, that I fix it here. I just note, that it is a problem, solution of which requires separate decoders. > >> +local function decode_count(response) >> + return response[1] >> +end >> + >> @@ -336,7 +385,10 @@ local function create_transport(host, port, user, password, callback, >> -- Decode xrow.body[DATA] to Lua objects >> body, body_end_check = decode(body_rpos) >> assert(body_end == body_end_check, "invalid xrow length") >> - return res -- the length of xrow.body >> - elseif not err then >> - setmetatable(res, sequence_mt) >> - local postproc = method ~= 'eval' and method ~= 'call_17' >> - if postproc then >> - local tnew = box.tuple.new > > You removed this local variable from the decoder - it was here > for a reason. Please put it back in your implementation of decoder. Ok. @@ -68,8 +68,9 @@ local function decode_unique_tuple(response) end local function decode_select(response) setmetatable(response, sequence_mt) + local tnew box.tuple.new for i, v in pairs(response) do - response[i] = box.tuple.new(v) + response[i] = tnew(v) end return response end > >> diff --git a/test/box/net.box.result b/test/box/net.box.result >> index cf7b27f0b..6a3713fc0 100644 >> --- a/test/box/net.box.result >> +++ b/test/box/net.box.result >> @@ -416,9 +416,11 @@ cn.space.net_box_test_space:select({234}, { iterator = 'LT' }) >> ... >> cn.space.net_box_test_space:update({1}, { { '+', 2, 2 } }) >> --- >> +- null > > net.box calling convention should be the same as local box, if a > tuple is not found, it should return nothing, not null. > Ok. diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua index 0e2485f66..f6eef56fb 100644 --- a/src/box/lua/net_box.lua +++ b/src/box/lua/net_box.lua @@ -68,7 +68,7 @@ local function decode_get(response) end local function decode_select(response) setmetatable(response, sequence_mt) - local tnew box.tuple.new + local tnew = box.tuple.new for i, v in pairs(response) do response[i] = tnew(v) end @@ -1099,6 +1099,12 @@ function console_methods:eval(line, timeout) return res[1] or res end +local function nothing_or_data(value) + if value ~= nil then + return value + end +end + space_metatable = function(remote) local methods = {} @@ -1129,7 +1135,8 @@ space_metatable = function(remote) function methods:upsert(key, oplist, opts) check_space_arg(self, 'upsert') - return remote:_request('upsert', opts, self.id, key, oplist) + return nothing_or_data(remote:_request('upsert', opts, self.id, key, + oplist)) end function methods:get(key, opts) @@ -1167,8 +1174,8 @@ index_metatable = function(remote) if opts and opts.buffer then error("index:get() doesn't support `buffer` argument") end - return 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.id, + self.id, box.index.EQ, 0, 2, key)) end function methods:min(key, opts) @@ -1176,8 +1183,9 @@ index_metatable = function(remote) if opts and opts.buffer then error("index:min() doesn't support `buffer` argument") end - return 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.id, + self.id, box.index.GE, 0, 1, + key)) end function methods:max(key, opts) @@ -1185,8 +1193,9 @@ index_metatable = function(remote) if opts and opts.buffer then error("index:max() doesn't support `buffer` argument") end - return 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.id, + self.id, box.index.LE, 0, 1, + key)) end function methods:count(key, opts) @@ -1201,13 +1210,14 @@ index_metatable = function(remote) function methods:delete(key, opts) check_index_arg(self, 'delete') - return remote:_request('delete', opts, self.space.id, self.id, key) + return nothing_or_data(remote:_request('delete', opts, self.space.id, + self.id, key)) end function methods:update(key, oplist, opts) check_index_arg(self, 'update') - return remote:_request('update', opts, self.space.id, self.id, key, - oplist) + return nothing_or_data(remote:_request('update', opts, self.space.id, + self.id, key, oplist)) end return { __index = methods, __metatable = false } diff --git a/test/box/net.box.result b/test/box/net.box.result index 6a3713fc0..4fbc70ec6 100644 --- a/test/box/net.box.result +++ b/test/box/net.box.result @@ -416,11 +416,9 @@ cn.space.net_box_test_space:select({234}, { iterator = 'LT' }) ... cn.space.net_box_test_space:update({1}, { { '+', 2, 2 } }) --- -- null ... cn.space.net_box_test_space:delete{1} --- -- null ... cn.space.net_box_test_space:delete{2} --- @@ -428,7 +426,6 @@ cn.space.net_box_test_space:delete{2} ... cn.space.net_box_test_space:delete{2} --- -- null ... -- test one-based indexing in splice operation (see update.test.lua) cn.space.net_box_test_space:replace({10, 'abcde'}) @@ -757,15 +754,12 @@ remote_space:upsert({3}, {}, { timeout = 1e-9 }) ... remote_space:upsert({4}, {}) --- -- null ... remote_space:upsert({5}, {}, { timeout = 1.00 }) --- -- null ... remote_space:upsert({3}, {}) --- -- null ... remote_space:update({3}, {}, { timeout = 1e-9 }) --- @@ -987,15 +981,12 @@ _ = remote_pk:delete({5}) ... remote_space:get(0) --- -- null ... remote_space:get(1) --- -- null ... remote_space:get(2) --- -- null ... remote_space = nil --- @@ -1327,7 +1318,6 @@ c.space.test:select{} ... c.space.test:upsert({1, 2, 'nothing'}, {{'+', 2, 1}}) -- common update --- -- null ... c.space.test:select{} --- @@ -1335,7 +1325,6 @@ c.space.test:select{} ... c.space.test:upsert({2, 4, 'something'}, {{'+', 2, 1}}) -- insert --- -- null ... c.space.test:select{} --- @@ -1344,7 +1333,6 @@ c.space.test:select{} ... c.space.test:upsert({2, 4, 'nothing'}, {{'+', 3, 100500}}) -- wrong operation --- -- null ... c.space.test:select{} --- diff --git a/test/box/sql.result b/test/box/sql.result index 95f8da7dd..11a698850 100644 --- a/test/box/sql.result +++ b/test/box/sql.result @@ -105,7 +105,6 @@ space:select{1} -- xxx: update comes through, returns 0 rows affected space:update(1, {{'=', 2, 'I am a new tuple'}}) --- -- null ... -- nothing is selected, since nothing was there space:select{1} @@ -209,7 +208,6 @@ space:delete(0) ... space:delete(4294967295) --- -- null ... box.space.test:drop() ---