[tarantool-patches] Re: [PATCH] net.box: ignore absence of _vcollation
Alexander Turenko
alexander.turenko at tarantool.org
Tue Jul 30 13:58:47 MSK 2019
Thank you for the detailed overview and the ways to solve the problem!
> Talking of this issue: your situation is simpler, as I see now, when I dived a
> bit into the problem. You have nothing to do about the netbox object. Your target
> is the worker fiber only. And in such a case I don't see any magic solution that
> would shrink this patch https://github.com/tarantool/tarantool/commit/abc88ef8d38e21a6c7faf925a19105ce65d45467
> to a couple of lines. The only obvious thing you can do here is to avoid passing
> version id to iproto_auth_sm and iproto_schema_sm functions, and Alexander proposed.
> Just make it a global value here
> https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L232
> together with the others, if you still will need.
I found it is even simpler: we already have greeting in this context and can
use it directly.
> Talking of how to remove branching - it has nothing to do with netbox specific code,
> or GC, or anything application dependent. It is a pure technical matter. You have a
> classic problem already solved in box/lua/upgrade.lua. The problem is that you have a
> pipeline of versions, each adding, or removing, or changing something. And it is logical
> to have a pipeline of functions building that schema step by step. IMO, you should have
> several functions to fetch and to install schema.
>
> Foe example:
>
> function fetch_schema_2_2_1()
> ...
> end
>
> function fetch_schema_2_2_2()
> fetch_schema_2_2_1()
> -- + some v_collation things.
> end
>
> function fetch_schema_2_2_3()
> fetch_schema_2_2_3()
> --- + ...
> end
>
> And you have a global table where key is version id, value is a schema fetch
> function. Here
> https://github.com/tarantool/tarantool/commit/abc88ef8d38e21a6c7faf925a19105ce65d45467#diff-a9dd9be71f8af749f288abb6304a5185R733
> you take a fetcher from the table like 'fetcher = fetchers[version_id]', and just
> call it.
We'll either need to add a fetcher for each release or choose a last
that is not newer then a peer's version_id.
> All these encode selects
> https://github.com/tarantool/tarantool/commit/abc88ef8d38e21a6c7faf925a19105ce65d45467#diff-a9dd9be71f8af749f288abb6304a5185R754
> should move to these schema fetchers. Of course, it wouldn't be that simple. At least,
> because schema fetch requests are sent in parallel, as you can see here:
> https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L743-L748
> And still the idea is the same. Perhaps you will have not only fetcher for each
> version, but two functions: start_fetch/end_fetch, or begin_fetch/end_fetch. A
> first function sends a request (encode_select), a second one waits for a response.
> Or let these fetchers return an asynchronous id, by which you will wait for
> responses in one place. Don't know what to chose. If I were you, and I had time, I
> would try to implement these step-wise fetchers. They would be quite easy to
> support and to extend for other future schema changes.
The idea is interesting. This however would take some effort (several
days for me I guess) to experimenting and find a good variant. Sadly, it
seems I can not pay enough time to that.
> Perhaps, it will remove the branching. Up to you what to do, to Alexander, and to
> other managers. My only wish is to look at the final patch when it will be ready.
Please, look at the patch below or on
Totktonada/gh-4307-net-box-fix-schema-fetch branch if the time allows. It is
more or less the same as Roman's one, but I was charged by your great email and
tried to minimize the patch as possible.
----
commit fff809e6e3d253245a1c596f3c2d51ab983f3efe
Author: Alexander Turenko <alexander.turenko at tarantool.org>
Date: Tue Jul 30 03:38:22 2019 +0300
net.box: fix schema fetching from 1.10/2.1 servers
After 2.2.0-390-ga7c855e5b ("net.box: fetch '_vcollation' sysview into
the module") net.box fetches _vcollation view unconditionally, while the
view was added in 2.2.0-389-g3e3ef182f and, say, tarantool-1.10 and
tarantool-2.1 do not have it. This leads to a runtime error "Space '277'
does not exist" on a newer client that connects to an older server.
Now the view is fetched conditionally depending of a version of a
server: if it is above 2.2.1, then net.box will fetch it. Note: at the
time there are no release with a number above 2.2.1.
When _vcollation view is available, a collation in an index part will be
shown by its name (with 'collation' field), otherwise it will be shown
by its ID (in 'collation_id' field). For example:
Connect to tarantool 1.10:
| tarantool> connection = require('net.box').connect('localhost:3301')
| ---
| ...
|
| tarantool> connection.space.s.index.sk.parts
| ---
| - - type: string
| is_nullable: false
| collation_id: 2
| fieldno: 2
| ...
Connect to tarantool 2.2.1 (when it will be released):
| tarantool> connection = require('net.box').connect('localhost:3301')
| ---
| ...
|
| tarantool> connection.space.s.index.sk.parts
| ---
| - - type: string
| is_nullable: false
| collation: unicode_ci
| fieldno: 2
| ...
Fixes #4307.
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
@@ -84,6 +84,14 @@ local function decode_push(raw_data)
return response[IPROTO_DATA_KEY][1], raw_end
end
+local function version_id(major, minor, patch)
+ return bit.bor(bit.lshift(major, 16), bit.lshift(minor, 8), patch)
+end
+
+local function version_at_least(peer_version_id, major, minor, patch)
+ return peer_version_id >= version_id(major, minor, patch)
+end
+
local method_encoder = {
ping = internal.encode_ping,
call_16 = internal.encode_call_16,
@@ -735,17 +743,23 @@ 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
+ local peer_has_vcollation = version_at_least(greeting.version_id,
+ 2, 2, 1)
local select1_id = new_request_id()
local select2_id = new_request_id()
- local select3_id = new_request_id()
+ local select3_id
local response = {}
-- fetch everything from space _vspace, 2 = ITER_ALL
encode_select(send_buf, select1_id, VSPACE_ID, 0, 2, 0, 0xFFFFFFFF, nil)
-- fetch everything from space _vindex, 2 = ITER_ALL
encode_select(send_buf, select2_id, VINDEX_ID, 0, 2, 0, 0xFFFFFFFF, nil)
-- fetch everything from space _vcollation, 2 = ITER_ALL
- encode_select(send_buf, select3_id, VCOLLATION_ID, 0, 2, 0, 0xFFFFFFFF,
- nil)
+ if peer_has_vcollation then
+ select3_id = new_request_id()
+ encode_select(send_buf, select3_id, VCOLLATION_ID, 0, 2, 0,
+ 0xFFFFFFFF, nil)
+ end
schema_version = nil -- any schema_version will do provided that
-- it is consistent across responses
@@ -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
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]
@@ -774,9 +790,10 @@ local function create_transport(host, port, user, password, callback,
response[id] = body[IPROTO_DATA_KEY]
end
until response[select1_id] and response[select2_id] and
- response[select3_id]
+ (not peer_has_vcollation or response[select3_id])
+ -- 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])
+ response[select2_id], response[select3_id])
set_state('active')
return iproto_sm(schema_version)
end
@@ -1269,17 +1286,23 @@ 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
local pkcollation = nil
- if pkcollationid ~= nil then
+ if pkcollationid ~= nil and collations ~= nil then
pkcollation = collations[pkcollationid + 1][2]
end
local pk = {
type = pktype,
fieldno = pkfield + 1,
- collation = pkcollation,
is_nullable = pknullable
}
+ if collations == nil then
+ pk.collation_id = pkcollationid
+ else
+ pk.collation = pkcollation
+ end
idx.parts[k] = pk
end
idx.unique = not not index[OPTS].unique
diff --git a/test/box/net.box.result b/test/box/net.box.result
index 1d9f43400..e3dabf7d9 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -2905,18 +2905,41 @@ c = net:connect(box.cfg.listen)
box.internal.collation.create('test', 'ICU', 'ru-RU')
---
...
+collation_id = box.internal.collation.id_by_name('test')
+---
+...
_ = space:create_index('sk', { type = 'tree', parts = {{1, 'str', collation = 'test'}}, unique = true })
---
...
c:reload_schema()
---
...
-c.space.test.index.sk.parts
+parts = c.space.test.index.sk.parts
---
-- - type: string
- is_nullable: false
- collation: test
- fieldno: 1
+...
+#parts == 1
+---
+- true
+...
+parts[1].fieldno == 1
+---
+- true
+...
+parts[1].type == 'string'
+---
+- true
+...
+parts[1].is_nullable == false
+---
+- true
+...
+if _TARANTOOL >= '2.2.1' then \
+ return parts[1].collation == 'test' \
+else \
+ return parts[1].collation_id == collation_id \
+end
+---
+- true
...
c:close()
---
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index de629ab59..8e65ff470 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1195,9 +1195,19 @@ c:close()
box.schema.user.grant('guest', 'read', 'space', 'test')
c = net:connect(box.cfg.listen)
box.internal.collation.create('test', 'ICU', 'ru-RU')
+collation_id = box.internal.collation.id_by_name('test')
_ = space:create_index('sk', { type = 'tree', parts = {{1, 'str', collation = 'test'}}, unique = true })
c:reload_schema()
-c.space.test.index.sk.parts
+parts = c.space.test.index.sk.parts
+#parts == 1
+parts[1].fieldno == 1
+parts[1].type == 'string'
+parts[1].is_nullable == false
+if _TARANTOOL >= '2.2.1' then \
+ return parts[1].collation == 'test' \
+else \
+ return parts[1].collation_id == collation_id \
+end
c:close()
box.internal.collation.drop('test')
space:drop()
diff --git a/test/box/stat_net.result b/test/box/stat_net.result
index 9eeada7ec..2cf567d57 100644
--- a/test/box/stat_net.result
+++ b/test/box/stat_net.result
@@ -83,9 +83,9 @@ box.stat.net.CONNECTIONS.total
---
- 4
...
-box.stat.net.REQUESTS.total
+box.stat.net.REQUESTS.total > 0
---
-- 17
+- true
...
box.stat.net.CONNECTIONS.current
---
@@ -115,6 +115,9 @@ test_run:wait_cond(function() return box.stat.net.CONNECTIONS.current == 1 end,
---
- true
...
+requests_total_saved = box.stat.net.REQUESTS.total
+---
+...
future1 = cn:call('tweedledee', {}, {is_async = true})
---
...
@@ -149,9 +152,9 @@ test_run:wait_cond(function() return box.stat.net.REQUESTS.current == 0 end, WAI
---
- true
...
-box.stat.net.REQUESTS.total
+box.stat.net.REQUESTS.total - requests_total_saved == 2
---
-- 19
+- true
...
-- reset
box.stat.reset()
diff --git a/test/box/stat_net.test.lua b/test/box/stat_net.test.lua
index 830deb36b..a1dd324bd 100644
--- a/test/box/stat_net.test.lua
+++ b/test/box/stat_net.test.lua
@@ -29,7 +29,7 @@ cn.space.tweedledum:select() --small request
box.stat.net.SENT.total > 0
box.stat.net.RECEIVED.total > 0
box.stat.net.CONNECTIONS.total
-box.stat.net.REQUESTS.total
+box.stat.net.REQUESTS.total > 0
box.stat.net.CONNECTIONS.current
box.stat.net.REQUESTS.current
@@ -41,6 +41,7 @@ test_run:wait_cond(function() return box.stat.net.CONNECTIONS.current == 2 end,
cn3:close()
test_run:wait_cond(function() return box.stat.net.CONNECTIONS.current == 1 end, WAIT_COND_TIMEOUT)
+requests_total_saved = box.stat.net.REQUESTS.total
future1 = cn:call('tweedledee', {}, {is_async = true})
test_run:wait_cond(function() return box.stat.net.REQUESTS.current == 1 end, WAIT_COND_TIMEOUT)
future2 = cn:call('tweedledee', {}, {is_async = true})
@@ -50,7 +51,7 @@ ch:put(true)
future1:wait_result()
future2:wait_result()
test_run:wait_cond(function() return box.stat.net.REQUESTS.current == 0 end, WAIT_COND_TIMEOUT)
-box.stat.net.REQUESTS.total
+box.stat.net.REQUESTS.total - requests_total_saved == 2
-- reset
box.stat.reset()
More information about the Tarantool-patches
mailing list