[tarantool-patches] Re: [PATCH] net.box: ignore absence of _vcollation

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Sat, Jul 20, 2019 at 10:07:14PM +0300, Alexander Turenko wrote:
> > > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > > index 2892bb3d3..723e63045 100644
> > > --- a/src/box/lua/net_box.lua
> > > +++ b/src/box/lua/net_box.lua
> > > @@ -761,6 +761,11 @@ local function create_transport(host, port, user, password, callback,
> > >                  if status ~= 0 then
> > >                      local body
> > >                      body, body_end = decode(body_rpos)
> > > +                    local err_msg = 'Space \'277\' does not exist'
> > 
> > Space 277? Seriously? This code handles not only collations request.
> > Should be more generic, IMHO.
> 
> Of course, it is better to use VCOLLATION_ID constant that already
> defined above.
> 
> Kirill, I didn't got your comment. This error message is only about one
> specific case: there is no such space. I think this approach is better
> then version check (that is proposed in the next version of the patch),
> because of readability: the version check approach leads to many small
> if's, while here we add only one check.
> 
> BTW, Kostja also has a comment:
> https://github.com/tarantool/tarantool/commit/47227cfa4c9bac76a882464a2d0440e5137f158e#commitcomment-34342515
> 
> WBR, Alexander Turenko.

We discussed two approaches with Kostja:

* Ignore _vcollation absense on a peer during schema (re)load:
  https://github.com/tarantool/tarantool/commit/47227cfa4c9bac76a882464a2d0440e5137f158e
* Don't fire a request according to a peer tarantool version:
  https://github.com/tarantool/tarantool/commit/abc88ef8d38e21a6c7faf925a19105ce65d45467

Summary of the discussion:

* The former lean on the fact that the only reason of the error re
  absense of _vcollation is an old version of a server. There is no
  guarantee that this assumption will retain in the future.
* The former approach checks an error message against a pattern.
  Changing of the error message will break backward compatibility.
* The latter approach looks bigger then the former one, because of
  passthrough of remote.peer_version_id value and more code with
  branching. We should try to find better way to implement this
  approach.
* Kostja also mentioned possibility to return 'collation' field always,
  even for old peers: just use a table with default collation_id -
  collation (string) mapping. It will not work with non-standard
  collations, but maybe it worth to consider this approach anyway.

To be honest I don't know net.box code enough to propose better way to
implement 2nd approach. Vlad, it would be great if you will look into
this and give some pointers to Roman, if time allows.

CCed Kostja and Vlad.

WBR, Alexander Turenko.