[tarantool-patches] Re: [PATCH 4/8] netbox: extend codec with 'decode' methods
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue May 8 20:24:33 MSK 2018
Hello. Thanks for review.
On 08/05/2018 18:49, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [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()
---
More information about the Tarantool-patches
mailing list