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 5336525D39 for ; Tue, 30 Jul 2019 18:35:32 -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 JwSvAR_ixy96 for ; Tue, 30 Jul 2019 18:35:32 -0400 (EDT) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 103CB25D12 for ; Tue, 30 Jul 2019 18:35:31 -0400 (EDT) Date: Wed, 31 Jul 2019 01:35:15 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] net.box: ignore absence of _vcollation Message-ID: <20190730223452.2vgivkmrptkslplc@tkn_work_nb> References: <20190708121338.76309-1-roman.habibov@tarantool.org> <20190712095549.ac4hhaxhxzgaik5i@tarantool.org> <20190720190714.5pejxoy2ae43nssp@tkn_work_nb> <20190724212743.jwrti2iuclleogjq@tkn_work_nb> <20190730105847.5bfgdhqqcxoqz7bh@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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: Kirill Yukhin Cc: Roman Khabibov , tarantool-patches@freelists.org, Konstantin Osipov , Vladislav Shpilevoy 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]