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

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

Kirill, I have acquired LGTMs from Kostja and Vlad (with the tiny comment
below, which I had fixed). Can you proceed with the change?

https://github.com/tarantool/tarantool/issues/4307
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4307-net-box-fix-schema-fetch

WBR, Alexander Turenko.

> > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > index 2892bb3d3..31a8c16b7 100644
> > --- a/src/box/lua/net_box.lua
> > +++ b/src/box/lua/net_box.lua
> > @@ -754,6 +768,8 @@ local function create_transport(host, port, user, password, callback,
> >              if err then return error_sm(err, hdr) end
> >              dispatch_response_iproto(hdr, body_rpos, body_end)
> >              local id = hdr[IPROTO_SYNC_KEY]
> > +            -- trick: omit check for peer_has_vcollation: id is
> > +            -- not nil
> 
> Nit: usually we start comments with a capital letter and finish
> with a dot. Here and in other hunks. Other than that the patch
> LGTM.

I'm aware of that. I gave glance to comments around and they are
formatted from a small letter and w/o a period, so I formatted the new
code in the same way to make reading more comfortable. Now hovewer I see
that there is no consistency in this point across net_box.lua file. So I
agree with you: it worth to write new comments in our standard way.

Applied an incremental diff at end of the email.

I have applied the diff, rebased the branch upward current master and
force-pushed the branch.

> > +if _TARANTOOL >= '2.2.1' then                    \
> > +    return parts[1].collation == 'test'          \
> > +else                                             \
> > +    return parts[1].collation_id == collation_id \
> 
> I like this new test-run feature with '\', looks way better
> than setopt delimited, and looks like C.

Thanks!

----

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 31a8c16b7..ea169637d 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -743,7 +743,7 @@ local function create_transport(host, port, user, password, callback,
             set_state('active')
             return iproto_sm(schema_version)
         end
-        -- _vcollation view was added in 2.2.0-389-g3e3ef182f
+        -- _vcollation view was added in 2.2.0-389-g3e3ef182f.
         local peer_has_vcollation = version_at_least(greeting.version_id,
                                                      2, 2, 1)
         local select1_id = new_request_id()
@@ -768,8 +768,8 @@ local function create_transport(host, port, user, password, callback,
             if err then return error_sm(err, hdr) end
             dispatch_response_iproto(hdr, body_rpos, body_end)
             local id = hdr[IPROTO_SYNC_KEY]
-            -- trick: omit check for peer_has_vcollation: id is
-            -- not nil
+            -- Trick: omit check for peer_has_vcollation: id is
+            -- not nil.
             if id == select1_id or id == select2_id or id == select3_id then
                 -- response to a schema query we've submitted
                 local status = hdr[IPROTO_STATUS_KEY]
@@ -791,7 +791,7 @@ local function create_transport(host, port, user, password, callback,
             end
         until response[select1_id] and response[select2_id] and
               (not peer_has_vcollation or response[select3_id])
-        -- trick: response[select3_id] is nil when the key is nil
+        -- Trick: response[select3_id] is nil when the key is nil.
         callback('did_fetch_schema', schema_version, response[select1_id],
                  response[select2_id], response[select3_id])
         set_state('active')
@@ -1286,8 +1286,8 @@ function remote_methods:_install_schema(schema_version, spaces, indices,
                 local pkcollationid = index[PARTS][k].collation
                 local pktype = index[PARTS][k][2] or index[PARTS][k].type
                 local pkfield = index[PARTS][k][1] or index[PARTS][k].field
-                -- resolve a collation name if a peer has
-                -- _vcollation view
+                -- Resolve a collation name if a peer has
+                -- _vcollation view.
                 local pkcollation = nil
                 if pkcollationid ~= nil and collations ~= nil then
                     pkcollation = collations[pkcollationid + 1][2]