[tarantool-patches] Re: [PATCH 4/8] netbox: extend codec with 'decode' methods

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for review.

On 08/05/2018 18:49, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@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()
  ---