* [Tarantool-patches] [PATCH vshard 1/7] test: print errors in a portable way
2020-05-01 0:16 [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy
@ 2020-05-01 0:16 ` Vladislav Shpilevoy
2020-05-01 16:58 ` Oleg Babin
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 2/7] router: introduce discovery_mode Vladislav Shpilevoy
` (6 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-01 0:16 UTC (permalink / raw)
To: tarantool-patches, olegrok
In 2.4.1 error objects have new 'base_type' field. That makes
number of unstable fields 2: 'trace' and 'base_type'. The latter
is unstable, because does not exist before 2.4.1.
The patch introduces a function which trims all not interesting
fields from error objects in the tests. That makes the test work
on all supported tarantools.
---
test/lua_libs/util.lua | 9 +++++++
test/misc/check_uuid_on_connect.result | 20 ++++++---------
test/misc/check_uuid_on_connect.test.lua | 7 ++---
test/rebalancer/bucket_ref.result | 8 +++---
test/rebalancer/bucket_ref.test.lua | 3 ++-
test/rebalancer/receiving_bucket.result | 18 +++++--------
test/rebalancer/receiving_bucket.test.lua | 8 +++---
test/router/retry_reads.result | 10 ++------
test/router/retry_reads.test.lua | 6 ++---
test/router/router.result | 31 ++++++++---------------
test/router/router.test.lua | 11 +++-----
test/router/sync.result | 15 ++++++-----
test/router/sync.test.lua | 4 ++-
test/storage/storage.result | 9 +++----
test/storage/storage.test.lua | 3 ++-
test/unit/error.result | 18 ++++++++-----
test/unit/error.test.lua | 9 +++++--
17 files changed, 91 insertions(+), 98 deletions(-)
diff --git a/test/lua_libs/util.lua b/test/lua_libs/util.lua
index fd7ed93..9c2e667 100644
--- a/test/lua_libs/util.lua
+++ b/test/lua_libs/util.lua
@@ -193,6 +193,14 @@ local function git_checkout(dst_dir, version)
return vshard_copy_path
end
+-- Portable representation of an error, not depending on Tarantool
+-- version and on any additional fields it can add. Trace is also
+-- trimmed in order for the tests not to depend on line numbers of
+-- the source files, which may slip into a .result file.
+local function portable_error(err)
+ return {code = err.code, type = err.type, message = err.message}
+end
+
return {
check_error = check_error,
shuffle_masters = shuffle_masters,
@@ -206,4 +214,5 @@ return {
SOURCEDIR = SOURCEDIR,
BUILDDIR = BUILDDIR,
git_checkout = git_checkout,
+ portable_error = portable_error,
}
diff --git a/test/misc/check_uuid_on_connect.result b/test/misc/check_uuid_on_connect.result
index d65aefe..6ebc5d0 100644
--- a/test/misc/check_uuid_on_connect.result
+++ b/test/misc/check_uuid_on_connect.result
@@ -4,10 +4,6 @@ test_run = require('test_run').new()
fiber = require('fiber')
---
...
-test_run:cmd("push filter 'line: *[0-9]+' to 'line: <line>'")
----
-- true
-...
REPLICASET_1 = { 'bad_uuid_1_a', 'bad_uuid_1_b' }
---
...
@@ -42,15 +38,15 @@ vshard.storage.bucket_force_create(1)
...
-- Fail, because replicaset_1 sees not the actual replicaset_2's
-- master UUID.
-vshard.storage.bucket_send(1, replicaset_uuid[2])
+res, err = vshard.storage.bucket_send(1, replicaset_uuid[2])
+---
+...
+res, util.portable_error(err)
---
- null
- type: ClientError
code: 77
message: Connection closed
- trace:
- - file: builtin/box/net_box.lua
- line: <line>
...
test_run:grep_log('bad_uuid_1_a', 'Mismatch server UUID on replica bad_uuid_2_a%(storage%@')
---
@@ -171,15 +167,15 @@ vshard.storage.bucket_force_create(2)
---
- true
...
-vshard.storage.bucket_send(2, replicaset_uuid[2])
+res, err = vshard.storage.bucket_send(2, replicaset_uuid[2])
+---
+...
+res, util.portable_error(err)
---
- null
- type: ClientError
code: 77
message: Connection closed
- trace:
- - file: builtin/box/net_box.lua
- line: <line>
...
-- Close existing connection on a first error and log it.
test_run:grep_log('bad_uuid_1_a', 'Mismatch server UUID on replica bad_uuid_2_a') ~= nil
diff --git a/test/misc/check_uuid_on_connect.test.lua b/test/misc/check_uuid_on_connect.test.lua
index b568c73..62f6593 100644
--- a/test/misc/check_uuid_on_connect.test.lua
+++ b/test/misc/check_uuid_on_connect.test.lua
@@ -1,6 +1,5 @@
test_run = require('test_run').new()
fiber = require('fiber')
-test_run:cmd("push filter 'line: *[0-9]+' to 'line: <line>'")
REPLICASET_1 = { 'bad_uuid_1_a', 'bad_uuid_1_b' }
REPLICASET_2 = { 'bad_uuid_2_a', 'bad_uuid_2_b' }
@@ -16,7 +15,8 @@ util = require('util')
vshard.storage.bucket_force_create(1)
-- Fail, because replicaset_1 sees not the actual replicaset_2's
-- master UUID.
-vshard.storage.bucket_send(1, replicaset_uuid[2])
+res, err = vshard.storage.bucket_send(1, replicaset_uuid[2])
+res, util.portable_error(err)
test_run:grep_log('bad_uuid_1_a', 'Mismatch server UUID on replica bad_uuid_2_a%(storage%@')
box.space._bucket:select{}
-- Bucket sending fails, but it remains 'sending'. It is because
@@ -64,7 +64,8 @@ util.wait_master(test_run, REPLICASET_2, 'bad_uuid_2_a')
test_run:switch('bad_uuid_1_a')
vshard.storage.bucket_force_create(2)
-vshard.storage.bucket_send(2, replicaset_uuid[2])
+res, err = vshard.storage.bucket_send(2, replicaset_uuid[2])
+res, util.portable_error(err)
-- Close existing connection on a first error and log it.
test_run:grep_log('bad_uuid_1_a', 'Mismatch server UUID on replica bad_uuid_2_a') ~= nil
diff --git a/test/rebalancer/bucket_ref.result b/test/rebalancer/bucket_ref.result
index b1f1b15..b66e449 100644
--- a/test/rebalancer/bucket_ref.result
+++ b/test/rebalancer/bucket_ref.result
@@ -137,15 +137,15 @@ vshard.storage.internal.errinj.ERRINJ_LONG_RECEIVE = true
_ = test_run:switch('box_1_a')
---
...
-vshard.storage.bucket_send(1, util.replicasets[2])
+res, err = vshard.storage.bucket_send(1, util.replicasets[2])
+---
+...
+res, util.portable_error(err)
---
- null
- type: ClientError
code: 32
message: Timeout exceeded
- trace:
- - file: '[C]'
- line: 4294967295
...
vshard.storage.buckets_info(1)
---
diff --git a/test/rebalancer/bucket_ref.test.lua b/test/rebalancer/bucket_ref.test.lua
index 1fe4d84..49ba583 100644
--- a/test/rebalancer/bucket_ref.test.lua
+++ b/test/rebalancer/bucket_ref.test.lua
@@ -48,7 +48,8 @@ vshard.storage.buckets_info(1)
_ = test_run:switch('box_2_a')
vshard.storage.internal.errinj.ERRINJ_LONG_RECEIVE = true
_ = test_run:switch('box_1_a')
-vshard.storage.bucket_send(1, util.replicasets[2])
+res, err = vshard.storage.bucket_send(1, util.replicasets[2])
+res, util.portable_error(err)
vshard.storage.buckets_info(1)
vshard.storage.bucket_ref(1, 'write')
vshard.storage.bucket_unref(1, 'write') -- Error, no refs.
diff --git a/test/rebalancer/receiving_bucket.result b/test/rebalancer/receiving_bucket.result
index 954b549..db6a67f 100644
--- a/test/rebalancer/receiving_bucket.result
+++ b/test/rebalancer/receiving_bucket.result
@@ -160,15 +160,15 @@ vshard.storage.internal.errinj.ERRINJ_RECEIVE_PARTIALLY = true
_ = test_run:switch('box_2_a')
---
...
-vshard.storage.bucket_send(1, util.replicasets[1], {timeout = 10})
+res, err = vshard.storage.bucket_send(1, util.replicasets[1], {timeout = 10})
+---
+...
+res, util.portable_error(err)
---
- null
- type: ClientError
code: 32
message: Error injection 'the bucket is received partially'
- trace:
- - file: '[C]'
- line: 4294967295
...
box.space._bucket:get{1}
---
@@ -222,10 +222,7 @@ _ = test_run:switch('box_2_a')
_, err = vshard.storage.bucket_send(101, util.replicasets[1], {timeout = 0.1})
---
...
-err.trace = nil
----
-...
-err
+util.portable_error(err)
---
- type: ClientError
code: 78
@@ -331,15 +328,12 @@ vshard.storage.bucket_refrw(1)
while f1:status() ~= 'dead' do fiber.sleep(0.01) end
---
...
-ret, err
+ret, util.portable_error(err)
---
- null
- type: ClientError
code: 78
message: Timeout exceeded
- trace:
- - file: '[C]'
- line: 4294967295
...
finish_long_thing = true
---
diff --git a/test/rebalancer/receiving_bucket.test.lua b/test/rebalancer/receiving_bucket.test.lua
index 4885815..1819cbb 100644
--- a/test/rebalancer/receiving_bucket.test.lua
+++ b/test/rebalancer/receiving_bucket.test.lua
@@ -62,7 +62,8 @@ _ = test_run:switch('box_1_a')
while box.space._bucket:get{1} do fiber.sleep(0.01) end
vshard.storage.internal.errinj.ERRINJ_RECEIVE_PARTIALLY = true
_ = test_run:switch('box_2_a')
-vshard.storage.bucket_send(1, util.replicasets[1], {timeout = 10})
+res, err = vshard.storage.bucket_send(1, util.replicasets[1], {timeout = 10})
+res, util.portable_error(err)
box.space._bucket:get{1}
_ = test_run:switch('box_1_a')
box.space._bucket:get{1}
@@ -88,8 +89,7 @@ _ = test_run:switch('box_1_a')
vshard.storage.internal.errinj.ERRINJ_LAST_RECEIVE_DELAY = true
_ = test_run:switch('box_2_a')
_, err = vshard.storage.bucket_send(101, util.replicasets[1], {timeout = 0.1})
-err.trace = nil
-err
+util.portable_error(err)
box.space._bucket:get{101}
while box.space._bucket:get{101}.status ~= vshard.consts.BUCKET.ACTIVE do vshard.storage.recovery_wakeup() fiber.sleep(0.01) end
box.space._bucket:get{101}
@@ -126,7 +126,7 @@ while f1:status() ~= 'suspended' do fiber.sleep(0.01) end
vshard.storage.buckets_info(1)
vshard.storage.bucket_refrw(1)
while f1:status() ~= 'dead' do fiber.sleep(0.01) end
-ret, err
+ret, util.portable_error(err)
finish_long_thing = true
while f:status() ~= 'dead' do fiber.sleep(0.01) end
vshard.storage.buckets_info(1)
diff --git a/test/router/retry_reads.result b/test/router/retry_reads.result
index 8873310..ae60267 100644
--- a/test/router/retry_reads.result
+++ b/test/router/retry_reads.result
@@ -113,10 +113,7 @@ fiber.time() - start < 1
---
- true
...
-e.trace = nil
----
-...
-e
+util.portable_error(e)
---
- type: ClientError
code: 0
@@ -125,10 +122,7 @@ e
_, e = rs1:callro('sleep', {1}, {timeout = 0.0001})
---
...
-e.trace = nil
----
-...
-e
+util.portable_error(e)
---
- type: ClientError
code: 78
diff --git a/test/router/retry_reads.test.lua b/test/router/retry_reads.test.lua
index ebfd1a9..c1fb689 100644
--- a/test/router/retry_reads.test.lua
+++ b/test/router/retry_reads.test.lua
@@ -40,12 +40,10 @@ fiber.time() - start < 1
start = fiber.time()
_, e = rs1:callro('raise_client_error', {}, {timeout = 5})
fiber.time() - start < 1
-e.trace = nil
-e
+util.portable_error(e)
_, e = rs1:callro('sleep', {1}, {timeout = 0.0001})
-e.trace = nil
-e
+util.portable_error(e)
--
-- Do not send multiple requests during timeout - it brokes long
diff --git a/test/router/router.result b/test/router/router.result
index 31a351f..d85f718 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -255,35 +255,21 @@ vshard.router.static.replicasets[util.replicasets[2]].bucket_count
_, e = vshard.router.callro(1, 'raise_client_error', {}, {})
---
...
-e.trace = nil
----
-...
-e
+util.portable_error(e)
---
- type: ClientError
- message: Unknown error
code: 32
-...
-tostring(e)
----
-- '{"type":"ClientError","message":"Unknown error","code":32}'
+ message: Unknown error
...
_, e = vshard.router.route(1):callro('raise_client_error', {})
---
...
-e.trace = nil
----
-...
-e
+util.portable_error(e)
---
- type: ClientError
code: 0
message: Unknown error
...
-tostring(e)
----
-- '{"type":"ClientError","code":0,"message":"Unknown error"}'
-...
-- Ensure, that despite not working multi-return, it is allowed
-- to return 'nil, err_obj'.
vshard.router.callro(1, 'echo', {nil, 'error_object'}, {})
@@ -632,11 +618,14 @@ future:is_ready()
future = vshard.router.callrw(bucket_id, 'raise_client_error', {}, {is_async = true})
---
...
-future:wait_result()
+res, err = future:wait_result()
---
-- null
-- {'type': 'ClientError', 'message': 'Unknown error', 'code': 32, 'trace': [{'file': '[C]',
- 'line': 4294967295}]}
+...
+util.portable_error(err)
+---
+- type: ClientError
+ code: 32
+ message: Unknown error
...
future:is_ready()
---
diff --git a/test/router/router.test.lua b/test/router/router.test.lua
index 571759a..abc1a3c 100644
--- a/test/router/router.test.lua
+++ b/test/router/router.test.lua
@@ -95,13 +95,9 @@ vshard.router.static.replicasets[util.replicasets[2]].bucket_count
-- Test lua errors.
--
_, e = vshard.router.callro(1, 'raise_client_error', {}, {})
-e.trace = nil
-e
-tostring(e)
+util.portable_error(e)
_, e = vshard.router.route(1):callro('raise_client_error', {})
-e.trace = nil
-e
-tostring(e)
+util.portable_error(e)
-- Ensure, that despite not working multi-return, it is allowed
-- to return 'nil, err_obj'.
vshard.router.callro(1, 'echo', {nil, 'error_object'}, {})
@@ -216,7 +212,8 @@ future = vshard.router.callro(bucket_id, 'space_get', {'test', {1}}, {is_async =
future:wait_result()
future:is_ready()
future = vshard.router.callrw(bucket_id, 'raise_client_error', {}, {is_async = true})
-future:wait_result()
+res, err = future:wait_result()
+util.portable_error(err)
future:is_ready()
future = vshard.router.callrw(bucket_id, 'do_push', args, {is_async = true})
func, iter, i = future:pairs()
diff --git a/test/router/sync.result b/test/router/sync.result
index 83443af..6f0821d 100644
--- a/test/router/sync.result
+++ b/test/router/sync.result
@@ -38,6 +38,9 @@ _ = test_run:cmd("start server router_1")
_ = test_run:switch("router_1")
---
...
+util = require('util')
+---
+...
vshard.router.bootstrap()
---
- true
@@ -47,15 +50,13 @@ vshard.router.sync(-1)
- null
- Timeout exceeded
...
-vshard.router.sync(0)
+res, err = vshard.router.sync(0)
---
-- null
-- replicaset: ac522f65-aa94-4134-9f64-51ee384f1a54
+...
+util.portable_error(err)
+---
+- type: ClientError
code: 78
- trace:
- - file: builtin/box/net_box.lua
- line: <line>
- type: ClientError
message: Timeout exceeded
...
--
diff --git a/test/router/sync.test.lua b/test/router/sync.test.lua
index 9602a4d..3150343 100644
--- a/test/router/sync.test.lua
+++ b/test/router/sync.test.lua
@@ -11,11 +11,13 @@ util.map_evals(test_run, {REPLICASET_1, REPLICASET_2}, 'bootstrap_storage(\'memt
_ = test_run:cmd("create server router_1 with script='router/router_1.lua'")
_ = test_run:cmd("start server router_1")
_ = test_run:switch("router_1")
+util = require('util')
vshard.router.bootstrap()
vshard.router.sync(-1)
-vshard.router.sync(0)
+res, err = vshard.router.sync(0)
+util.portable_error(err)
--
-- gh-190: router should not ignore cfg.sync_timeout.
diff --git a/test/storage/storage.result b/test/storage/storage.result
index 007d9fb..424bc4c 100644
--- a/test/storage/storage.result
+++ b/test/storage/storage.result
@@ -506,15 +506,14 @@ vshard.storage.bucket_recv(100, 'from_uuid', {{1000, {{1}}}})
--
-- Test not existing space in bucket data.
--
-vshard.storage.bucket_recv(4, util.replicasets[2], {{1000, {{1}}}})
+res, err = vshard.storage.bucket_recv(4, util.replicasets[2], {{1000, {{1}}}})
+---
+...
+util.portable_error(err)
---
-- null
- type: ClientError
code: 36
message: Space '1000' does not exist
- trace:
- - file: '[C]'
- line: 4294967295
...
while box.space._bucket:get{4} do vshard.storage.recovery_wakeup() fiber.sleep(0.01) end
---
diff --git a/test/storage/storage.test.lua b/test/storage/storage.test.lua
index 0e82cd1..d631b51 100644
--- a/test/storage/storage.test.lua
+++ b/test/storage/storage.test.lua
@@ -121,7 +121,8 @@ vshard.storage.bucket_recv(100, 'from_uuid', {{1000, {{1}}}})
--
-- Test not existing space in bucket data.
--
-vshard.storage.bucket_recv(4, util.replicasets[2], {{1000, {{1}}}})
+res, err = vshard.storage.bucket_recv(4, util.replicasets[2], {{1000, {{1}}}})
+util.portable_error(err)
while box.space._bucket:get{4} do vshard.storage.recovery_wakeup() fiber.sleep(0.01) end
--
diff --git a/test/unit/error.result b/test/unit/error.result
index f74438f..144df44 100644
--- a/test/unit/error.result
+++ b/test/unit/error.result
@@ -19,9 +19,15 @@ ok, err = pcall(box.error, box.error.TIMEOUT)
box_error = lerror.box(err)
---
...
-tostring(box_error)
+str = tostring(box_error)
---
-- '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}'
+...
+-- Base_type appears in 2.4.1. Simple nullification of base_type
+-- won't work, since order of the old fields becomes different.
+assert(str == '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}' or \
+ str == '{"code":78,"base_type":"ClientError","type":"ClientError","message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}')
+---
+- true
...
vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, 'uuid', 'reason')
---
@@ -80,12 +86,12 @@ function raise_lua_err() assert(false) end
ok, err = pcall(raise_lua_err)
---
...
-lerror.make(err)
+err = lerror.make(err)
+---
+...
+util.portable_error(err)
---
- type: ClientError
code: 32
message: '[string "function raise_lua_err() assert(false) end "]:1: assertion failed!'
- trace:
- - file: '[C]'
- line: 4294967295
...
diff --git a/test/unit/error.test.lua b/test/unit/error.test.lua
index 6c15707..2ba7da6 100644
--- a/test/unit/error.test.lua
+++ b/test/unit/error.test.lua
@@ -8,7 +8,11 @@ lerror = vshard.error
--
ok, err = pcall(box.error, box.error.TIMEOUT)
box_error = lerror.box(err)
-tostring(box_error)
+str = tostring(box_error)
+-- Base_type appears in 2.4.1. Simple nullification of base_type
+-- won't work, since order of the old fields becomes different.
+assert(str == '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}' or \
+ str == '{"code":78,"base_type":"ClientError","type":"ClientError","message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}')
vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, 'uuid', 'reason')
tostring(vshard_error)
@@ -32,4 +36,5 @@ util.check_error(lerror.vshard, 'Wrong format code', 'arg1', 'arg2')
function raise_lua_err() assert(false) end
ok, err = pcall(raise_lua_err)
-lerror.make(err)
+err = lerror.make(err)
+util.portable_error(err)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 1/7] test: print errors in a portable way
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 1/7] test: print errors in a portable way Vladislav Shpilevoy
@ 2020-05-01 16:58 ` Oleg Babin
2020-05-02 20:08 ` Vladislav Shpilevoy
0 siblings, 1 reply; 29+ messages in thread
From: Oleg Babin @ 2020-05-01 16:58 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Hi! Thanks for the patch! See three small comments.
On 01/05/2020 03:16, Vladislav Shpilevoy wrote:
>
> +-- Portable representation of an error, not depending on Tarantool
> +-- version and on any additional fields it can add. Trace is also
> +-- trimmed in order for the tests not to depend on line numbers of
> +-- the source files, which may slip into a .result file.
> +local function portable_error(err)
> + return {code = err.code, type = err.type, message = err.message}
> +end
I propose to add "nil" check here. At first it allows to use this hepler
not only when we expect error. Secondly this approach eliminates
confusing error messages if tests break for some reasons.
> +
> return {
> check_error = check_error,
> shuffle_masters = shuffle_masters,
> @@ -206,4 +214,5 @@ return {
> SOURCEDIR = SOURCEDIR,
> BUILDDIR = BUILDDIR,
> git_checkout = git_checkout,
> + portable_error = portable_error,
> }
> diff --git a/test/misc/check_uuid_on_connect.result b/test/misc/check_uuid_on_connect.result
> index d65aefe..6ebc5d0 100644
> --- a/test/misc/check_uuid_on_connect.result
> +++ b/test/misc/check_uuid_on_connect.result
> @@ -4,10 +4,6 @@ test_run = require('test_run').new()
> fiber = require('fiber')
> ---
> ...
> -test_run:cmd("push filter 'line: *[0-9]+' to 'line: <line>'")
> ----
> -- true
> -...
> REPLICASET_1 = { 'bad_uuid_1_a', 'bad_uuid_1_b' }
> ---
> ...
> @@ -42,15 +38,15 @@ vshard.storage.bucket_force_create(1)
> ...
> -- Fail, because replicaset_1 sees not the actual replicaset_2's
> -- master UUID.
> -vshard.storage.bucket_send(1, replicaset_uuid[2])
> +res, err = vshard.storage.bucket_send(1, replicaset_uuid[2])
> +---
> +...
> +res, util.portable_error(err)
> ---
> - null
> - type: ClientError
> code: 77
> message: Connection closed
> - trace:
> - - file: builtin/box/net_box.lua
> - line: <line>
> ...
> test_run:grep_log('bad_uuid_1_a', 'Mismatch server UUID on replica bad_uuid_2_a%(storage%@')
> ---
> @@ -171,15 +167,15 @@ vshard.storage.bucket_force_create(2)
> ---
> - true
> ...
> -vshard.storage.bucket_send(2, replicaset_uuid[2])
> +res, err = vshard.storage.bucket_send(2, replicaset_uuid[2])
> +---
> +...
> +res, util.portable_error(err)
> ---
> - null
> - type: ClientError
> code: 77
> message: Connection closed
> - trace:
> - - file: builtin/box/net_box.lua
> - line: <line>
> ...
> -- Close existing connection on a first error and log it.
> test_run:grep_log('bad_uuid_1_a', 'Mismatch server UUID on replica bad_uuid_2_a') ~= nil
> diff --git a/test/misc/check_uuid_on_connect.test.lua b/test/misc/check_uuid_on_connect.test.lua
> index b568c73..62f6593 100644
> --- a/test/misc/check_uuid_on_connect.test.lua
> +++ b/test/misc/check_uuid_on_connect.test.lua
> @@ -1,6 +1,5 @@
> test_run = require('test_run').new()
> fiber = require('fiber')
> -test_run:cmd("push filter 'line: *[0-9]+' to 'line: <line>'")
>
> REPLICASET_1 = { 'bad_uuid_1_a', 'bad_uuid_1_b' }
> REPLICASET_2 = { 'bad_uuid_2_a', 'bad_uuid_2_b' }
> @@ -16,7 +15,8 @@ util = require('util')
> vshard.storage.bucket_force_create(1)
> -- Fail, because replicaset_1 sees not the actual replicaset_2's
> -- master UUID.
> -vshard.storage.bucket_send(1, replicaset_uuid[2])
> +res, err = vshard.storage.bucket_send(1, replicaset_uuid[2])
> +res, util.portable_error(err)
> test_run:grep_log('bad_uuid_1_a', 'Mismatch server UUID on replica bad_uuid_2_a%(storage%@')
> box.space._bucket:select{}
> -- Bucket sending fails, but it remains 'sending'. It is because
> @@ -64,7 +64,8 @@ util.wait_master(test_run, REPLICASET_2, 'bad_uuid_2_a')
>
> test_run:switch('bad_uuid_1_a')
> vshard.storage.bucket_force_create(2)
> -vshard.storage.bucket_send(2, replicaset_uuid[2])
> +res, err = vshard.storage.bucket_send(2, replicaset_uuid[2])
> +res, util.portable_error(err)
> -- Close existing connection on a first error and log it.
> test_run:grep_log('bad_uuid_1_a', 'Mismatch server UUID on replica bad_uuid_2_a') ~= nil
>
> diff --git a/test/rebalancer/bucket_ref.result b/test/rebalancer/bucket_ref.result
> index b1f1b15..b66e449 100644
> --- a/test/rebalancer/bucket_ref.result
> +++ b/test/rebalancer/bucket_ref.result
> @@ -137,15 +137,15 @@ vshard.storage.internal.errinj.ERRINJ_LONG_RECEIVE = true
> _ = test_run:switch('box_1_a')
> ---
> ...
> -vshard.storage.bucket_send(1, util.replicasets[2])
> +res, err = vshard.storage.bucket_send(1, util.replicasets[2])
> +---
> +...
> +res, util.portable_error(err)
> ---
> - null
> - type: ClientError
> code: 32
> message: Timeout exceeded
> - trace:
> - - file: '[C]'
> - line: 4294967295
> ...
> vshard.storage.buckets_info(1)
> ---
> diff --git a/test/rebalancer/bucket_ref.test.lua b/test/rebalancer/bucket_ref.test.lua
> index 1fe4d84..49ba583 100644
> --- a/test/rebalancer/bucket_ref.test.lua
> +++ b/test/rebalancer/bucket_ref.test.lua
> @@ -48,7 +48,8 @@ vshard.storage.buckets_info(1)
> _ = test_run:switch('box_2_a')
> vshard.storage.internal.errinj.ERRINJ_LONG_RECEIVE = true
> _ = test_run:switch('box_1_a')
> -vshard.storage.bucket_send(1, util.replicasets[2])
> +res, err = vshard.storage.bucket_send(1, util.replicasets[2])
> +res, util.portable_error(err)
> vshard.storage.buckets_info(1)
> vshard.storage.bucket_ref(1, 'write')
> vshard.storage.bucket_unref(1, 'write') -- Error, no refs.
> diff --git a/test/rebalancer/receiving_bucket.result b/test/rebalancer/receiving_bucket.result
> index 954b549..db6a67f 100644
> --- a/test/rebalancer/receiving_bucket.result
> +++ b/test/rebalancer/receiving_bucket.result
> @@ -160,15 +160,15 @@ vshard.storage.internal.errinj.ERRINJ_RECEIVE_PARTIALLY = true
> _ = test_run:switch('box_2_a')
> ---
> ...
> -vshard.storage.bucket_send(1, util.replicasets[1], {timeout = 10})
> +res, err = vshard.storage.bucket_send(1, util.replicasets[1], {timeout = 10})
> +---
> +...
> +res, util.portable_error(err)
> ---
> - null
> - type: ClientError
> code: 32
> message: Error injection 'the bucket is received partially'
> - trace:
> - - file: '[C]'
> - line: 4294967295
> ...
> box.space._bucket:get{1}
> ---
> @@ -222,10 +222,7 @@ _ = test_run:switch('box_2_a')
> _, err = vshard.storage.bucket_send(101, util.replicasets[1], {timeout = 0.1})
> ---
> ...
> -err.trace = nil
> ----
> -...
> -err
> +util.portable_error(err)
> ---
> - type: ClientError
> code: 78
> @@ -331,15 +328,12 @@ vshard.storage.bucket_refrw(1)
> while f1:status() ~= 'dead' do fiber.sleep(0.01) end
> ---
> ...
> -ret, err
> +ret, util.portable_error(err)
> ---
> - null
> - type: ClientError
> code: 78
> message: Timeout exceeded
> - trace:
> - - file: '[C]'
> - line: 4294967295
> ...
> finish_long_thing = true
> ---
> diff --git a/test/rebalancer/receiving_bucket.test.lua b/test/rebalancer/receiving_bucket.test.lua
> index 4885815..1819cbb 100644
> --- a/test/rebalancer/receiving_bucket.test.lua
> +++ b/test/rebalancer/receiving_bucket.test.lua
> @@ -62,7 +62,8 @@ _ = test_run:switch('box_1_a')
> while box.space._bucket:get{1} do fiber.sleep(0.01) end
> vshard.storage.internal.errinj.ERRINJ_RECEIVE_PARTIALLY = true
> _ = test_run:switch('box_2_a')
> -vshard.storage.bucket_send(1, util.replicasets[1], {timeout = 10})
> +res, err = vshard.storage.bucket_send(1, util.replicasets[1], {timeout = 10})
> +res, util.portable_error(err)
> box.space._bucket:get{1}
> _ = test_run:switch('box_1_a')
> box.space._bucket:get{1}
> @@ -88,8 +89,7 @@ _ = test_run:switch('box_1_a')
> vshard.storage.internal.errinj.ERRINJ_LAST_RECEIVE_DELAY = true
> _ = test_run:switch('box_2_a')
> _, err = vshard.storage.bucket_send(101, util.replicasets[1], {timeout = 0.1})
> -err.trace = nil
> -err
> +util.portable_error(err)
> box.space._bucket:get{101}
> while box.space._bucket:get{101}.status ~= vshard.consts.BUCKET.ACTIVE do vshard.storage.recovery_wakeup() fiber.sleep(0.01) end
> box.space._bucket:get{101}
> @@ -126,7 +126,7 @@ while f1:status() ~= 'suspended' do fiber.sleep(0.01) end
> vshard.storage.buckets_info(1)
> vshard.storage.bucket_refrw(1)
> while f1:status() ~= 'dead' do fiber.sleep(0.01) end
> -ret, err
> +ret, util.portable_error(err)
> finish_long_thing = true
> while f:status() ~= 'dead' do fiber.sleep(0.01) end
> vshard.storage.buckets_info(1)
> diff --git a/test/router/retry_reads.result b/test/router/retry_reads.result
> index 8873310..ae60267 100644
> --- a/test/router/retry_reads.result
> +++ b/test/router/retry_reads.result
> @@ -113,10 +113,7 @@ fiber.time() - start < 1
> ---
> - true
> ...
> -e.trace = nil
> ----
> -...
> -e
> +util.portable_error(e)
> ---
> - type: ClientError
> code: 0
> @@ -125,10 +122,7 @@ e
> _, e = rs1:callro('sleep', {1}, {timeout = 0.0001})
> ---
> ...
> -e.trace = nil
> ----
> -...
> -e
> +util.portable_error(e)
> ---
> - type: ClientError
> code: 78
> diff --git a/test/router/retry_reads.test.lua b/test/router/retry_reads.test.lua
> index ebfd1a9..c1fb689 100644
> --- a/test/router/retry_reads.test.lua
> +++ b/test/router/retry_reads.test.lua
> @@ -40,12 +40,10 @@ fiber.time() - start < 1
> start = fiber.time()
> _, e = rs1:callro('raise_client_error', {}, {timeout = 5})
> fiber.time() - start < 1
> -e.trace = nil
> -e
> +util.portable_error(e)
>
> _, e = rs1:callro('sleep', {1}, {timeout = 0.0001})
> -e.trace = nil
> -e
> +util.portable_error(e)
>
> --
> -- Do not send multiple requests during timeout - it brokes long
> diff --git a/test/router/router.result b/test/router/router.result
> index 31a351f..d85f718 100644
> --- a/test/router/router.result
> +++ b/test/router/router.result
> @@ -255,35 +255,21 @@ vshard.router.static.replicasets[util.replicasets[2]].bucket_count
> _, e = vshard.router.callro(1, 'raise_client_error', {}, {})
> ---
> ...
> -e.trace = nil
> ----
> -...
> -e
> +util.portable_error(e)
> ---
> - type: ClientError
> - message: Unknown error
> code: 32
> -...
> -tostring(e)
> ----
> -- '{"type":"ClientError","message":"Unknown error","code":32}'
> + message: Unknown error
> ...
> _, e = vshard.router.route(1):callro('raise_client_error', {})
> ---
> ...
> -e.trace = nil
> ----
> -...
> -e
> +util.portable_error(e)
> ---
> - type: ClientError
> code: 0
> message: Unknown error
> ...
> -tostring(e)
> ----
> -- '{"type":"ClientError","code":0,"message":"Unknown error"}'
> -...
> -- Ensure, that despite not working multi-return, it is allowed
> -- to return 'nil, err_obj'.
> vshard.router.callro(1, 'echo', {nil, 'error_object'}, {})
> @@ -632,11 +618,14 @@ future:is_ready()
> future = vshard.router.callrw(bucket_id, 'raise_client_error', {}, {is_async = true})
> ---
> ...
> -future:wait_result()
> +res, err = future:wait_result()
> ---
> -- null
> -- {'type': 'ClientError', 'message': 'Unknown error', 'code': 32, 'trace': [{'file': '[C]',
> - 'line': 4294967295}]}
> +...
> +util.portable_error(err)
> +---
> +- type: ClientError
> + code: 32
> + message: Unknown error
> ...
> future:is_ready()
> ---
> diff --git a/test/router/router.test.lua b/test/router/router.test.lua
> index 571759a..abc1a3c 100644
> --- a/test/router/router.test.lua
> +++ b/test/router/router.test.lua
> @@ -95,13 +95,9 @@ vshard.router.static.replicasets[util.replicasets[2]].bucket_count
> -- Test lua errors.
> --
> _, e = vshard.router.callro(1, 'raise_client_error', {}, {})
> -e.trace = nil
> -e
> -tostring(e)
> +util.portable_error(e)
> _, e = vshard.router.route(1):callro('raise_client_error', {})
> -e.trace = nil
> -e
> -tostring(e)
> +util.portable_error(e)
> -- Ensure, that despite not working multi-return, it is allowed
> -- to return 'nil, err_obj'.
> vshard.router.callro(1, 'echo', {nil, 'error_object'}, {})
> @@ -216,7 +212,8 @@ future = vshard.router.callro(bucket_id, 'space_get', {'test', {1}}, {is_async =
> future:wait_result()
> future:is_ready()
> future = vshard.router.callrw(bucket_id, 'raise_client_error', {}, {is_async = true})
> -future:wait_result()
> +res, err = future:wait_result()
> +util.portable_error(err)
> future:is_ready()
> future = vshard.router.callrw(bucket_id, 'do_push', args, {is_async = true})
> func, iter, i = future:pairs()
> diff --git a/test/router/sync.result b/test/router/sync.result
> index 83443af..6f0821d 100644
> --- a/test/router/sync.result
> +++ b/test/router/sync.result
> @@ -38,6 +38,9 @@ _ = test_run:cmd("start server router_1")
> _ = test_run:switch("router_1")
> ---
> ...
> +util = require('util')
> +---
> +...
> vshard.router.bootstrap()
> ---
> - true
> @@ -47,15 +50,13 @@ vshard.router.sync(-1)
> - null
> - Timeout exceeded
> ...
> -vshard.router.sync(0)
> +res, err = vshard.router.sync(0)
> ---
> -- null
> -- replicaset: ac522f65-aa94-4134-9f64-51ee384f1a54
> +...
> +util.portable_error(err)
> +---
> +- type: ClientError
> code: 78
> - trace:
> - - file: builtin/box/net_box.lua
> - line: <line>
> - type: ClientError
> message: Timeout exceeded
> ...
> --
> diff --git a/test/router/sync.test.lua b/test/router/sync.test.lua
> index 9602a4d..3150343 100644
> --- a/test/router/sync.test.lua
> +++ b/test/router/sync.test.lua
> @@ -11,11 +11,13 @@ util.map_evals(test_run, {REPLICASET_1, REPLICASET_2}, 'bootstrap_storage(\'memt
> _ = test_run:cmd("create server router_1 with script='router/router_1.lua'")
> _ = test_run:cmd("start server router_1")
> _ = test_run:switch("router_1")
> +util = require('util')
>
> vshard.router.bootstrap()
>
> vshard.router.sync(-1)
> -vshard.router.sync(0)
> +res, err = vshard.router.sync(0)
> +util.portable_error(err)
>
> --
> -- gh-190: router should not ignore cfg.sync_timeout.
> diff --git a/test/storage/storage.result b/test/storage/storage.result
> index 007d9fb..424bc4c 100644
> --- a/test/storage/storage.result
> +++ b/test/storage/storage.result
> @@ -506,15 +506,14 @@ vshard.storage.bucket_recv(100, 'from_uuid', {{1000, {{1}}}})
> --
> -- Test not existing space in bucket data.
> --
> -vshard.storage.bucket_recv(4, util.replicasets[2], {{1000, {{1}}}})
> +res, err = vshard.storage.bucket_recv(4, util.replicasets[2], {{1000, {{1}}}})
> +---
> +...
> +util.portable_error(err)
> ---
> -- null
> - type: ClientError
> code: 36
> message: Space '1000' does not exist
> - trace:
> - - file: '[C]'
> - line: 4294967295
> ...
> while box.space._bucket:get{4} do vshard.storage.recovery_wakeup() fiber.sleep(0.01) end
> ---
> diff --git a/test/storage/storage.test.lua b/test/storage/storage.test.lua
> index 0e82cd1..d631b51 100644
> --- a/test/storage/storage.test.lua
> +++ b/test/storage/storage.test.lua
> @@ -121,7 +121,8 @@ vshard.storage.bucket_recv(100, 'from_uuid', {{1000, {{1}}}})
> --
> -- Test not existing space in bucket data.
> --
> -vshard.storage.bucket_recv(4, util.replicasets[2], {{1000, {{1}}}})
> +res, err = vshard.storage.bucket_recv(4, util.replicasets[2], {{1000, {{1}}}})
> +util.portable_error(err)
> while box.space._bucket:get{4} do vshard.storage.recovery_wakeup() fiber.sleep(0.01) end
>
> --
> diff --git a/test/unit/error.result b/test/unit/error.result
> index f74438f..144df44 100644
> --- a/test/unit/error.result
> +++ b/test/unit/error.result
> @@ -19,9 +19,15 @@ ok, err = pcall(box.error, box.error.TIMEOUT)
> box_error = lerror.box(err)
> ---
> ...
> -tostring(box_error)
> +str = tostring(box_error)
> ---
> -- '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}'
> +...
> +-- Base_type appears in 2.4.1. Simple nullification of base_type
> +-- won't work, since order of the old fields becomes different.
> +assert(str == '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}' or \
> + str == '{"code":78,"base_type":"ClientError","type":"ClientError","message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}')
> +---
> +- true
> ...
> vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, 'uuid', 'reason')
> ---
> @@ -80,12 +86,12 @@ function raise_lua_err() assert(false) end
> ok, err = pcall(raise_lua_err)
> ---
> ...
> -lerror.make(err)
> +err = lerror.make(err)
> +---
> +...
> +util.portable_error(err)
> ---
> - type: ClientError
> code: 32
> message: '[string "function raise_lua_err() assert(false) end "]:1: assertion failed!'
> - trace:
> - - file: '[C]'
> - line: 4294967295
> ...
> diff --git a/test/unit/error.test.lua b/test/unit/error.test.lua
> index 6c15707..2ba7da6 100644
> --- a/test/unit/error.test.lua
> +++ b/test/unit/error.test.lua
> @@ -8,7 +8,11 @@ lerror = vshard.error
> --
> ok, err = pcall(box.error, box.error.TIMEOUT)
> box_error = lerror.box(err)
Why do you use pcall(box.error, box.error.TIMEOUT)? Is it needed for
promote an error to box.error.last()? Why not simply
`box.error.new(box.error.TIMEOUT)?` I doesn't expect that it will
promote an error to box.error.last() (even if
https://github.com/tarantool/tarantool/issues/4778 still works in
1.10/2.2/2.3), but I think that this isn't really needed if you return
`nil, err`.
> -tostring(box_error)
> +str = tostring(box_error)
> +-- Base_type appears in 2.4.1. Simple nullification of base_type
> +-- won't work, since order of the old fields becomes different.
> +assert(str == '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}' or \
> + str == '{"code":78,"base_type":"ClientError","type":"ClientError","message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}')
>
I assume that we don't have a guarantee that our error will be printed
as `{"type":"ClientError","code":78 ... }` and not as `{"code":78,
"type":"ClientError" ...}`. Also there is confusing "line" parameter. Is
it stable for different versions? I propose to check it as
```
err.type == "ClientError"
err.code == 78
...
```
> vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, 'uuid', 'reason')
> tostring(vshard_error)
> @@ -32,4 +36,5 @@ util.check_error(lerror.vshard, 'Wrong format code', 'arg1', 'arg2')
>
> function raise_lua_err() assert(false) end
> ok, err = pcall(raise_lua_err)
> -lerror.make(err)
> +err = lerror.make(err)
> +util.portable_error(err)
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 1/7] test: print errors in a portable way
2020-05-01 16:58 ` Oleg Babin
@ 2020-05-02 20:08 ` Vladislav Shpilevoy
2020-05-04 14:26 ` Oleg Babin
0 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-02 20:08 UTC (permalink / raw)
To: Oleg Babin, tarantool-patches
Hi! Thanks for the review!
> On 01/05/2020 03:16, Vladislav Shpilevoy wrote:
>> +-- Portable representation of an error, not depending on Tarantool
>> +-- version and on any additional fields it can add. Trace is also
>> +-- trimmed in order for the tests not to depend on line numbers of
>> +-- the source files, which may slip into a .result file.
>> +local function portable_error(err)
>> + return {code = err.code, type = err.type, message = err.message}
>> +end
>
> I propose to add "nil" check here. At first it allows to use this hepler not only when we expect error. Secondly this approach eliminates confusing error messages if tests break for some reasons.
But this function is supposed to be used only on errors. So
it looks strange to put here any workarounds for non-errors.
>> diff --git a/test/unit/error.test.lua b/test/unit/error.test.lua
>> index 6c15707..2ba7da6 100644
>> --- a/test/unit/error.test.lua
>> +++ b/test/unit/error.test.lua
>> @@ -8,7 +8,11 @@ lerror = vshard.error
>> --
>> ok, err = pcall(box.error, box.error.TIMEOUT)
>> box_error = lerror.box(err)
>
> Why do you use pcall(box.error, box.error.TIMEOUT)? Is it needed for promote an error to box.error.last()? Why not simply `box.error.new(box.error.TIMEOUT)?` I doesn't expect that it will promote an error to box.error.last() (even if https://github.com/tarantool/tarantool/issues/4778 still works in 1.10/2.2/2.3), but I think that this isn't really needed if you return `nil, err`.
The lines you are referring to were written for 1.9 version.
box.error.new() does not exist in 1.9. Since then there was
no reason to change these particular test code lines.
>> -tostring(box_error)
>> +str = tostring(box_error)
>> +-- Base_type appears in 2.4.1. Simple nullification of base_type
>> +-- won't work, since order of the old fields becomes different.
>> +assert(str == '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}' or \
>> + str == '{"code":78,"base_type":"ClientError","type":"ClientError","message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}')
>>
>
> I assume that we don't have a guarantee that our error will be printed as `{"type":"ClientError","code":78 ... }` and not as `{"code":78, "type":"ClientError" ...}`.
Actually we have. If you insert the same keys in the same order,
resulting iteration order will be the same everywhere (at least
this is what I always observed). Unless luajit will change the
way how it places values into tables, or if a new key is added.
The latter is exactly what happened because of 'base_type'
in 2.4.1.
> Also there is confusing "line" parameter. Is it stable for different versions? I propose to check it as
> ```
> err.type == "ClientError"
> err.code == 78
> ...
> ```
The 'line' is stable. It is UINT32_MAX when unknown.
The test is exactly about string representation. That it is json.
Removing check of how the string looks would make the test useless.
However I found another workaround:
====================
diff --git a/test/unit/error.result b/test/unit/error.result
index 144df44..8552d91 100644
--- a/test/unit/error.result
+++ b/test/unit/error.result
@@ -7,6 +7,9 @@ vshard = require('vshard')
util = require('util')
---
...
+json = require('json')
+---
+...
lerror = vshard.error
---
...
@@ -22,12 +25,11 @@ box_error = lerror.box(err)
str = tostring(box_error)
---
...
--- Base_type appears in 2.4.1. Simple nullification of base_type
--- won't work, since order of the old fields becomes different.
-assert(str == '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}' or \
- str == '{"code":78,"base_type":"ClientError","type":"ClientError","message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}')
+util.portable_error(json.decode(str))
---
-- true
+- type: ClientError
+ code: 78
+ message: Timeout exceeded
...
vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, 'uuid', 'reason')
---
diff --git a/test/unit/error.test.lua b/test/unit/error.test.lua
index 2ba7da6..859414e 100644
--- a/test/unit/error.test.lua
+++ b/test/unit/error.test.lua
@@ -1,6 +1,7 @@
test_run = require('test_run').new()
vshard = require('vshard')
util = require('util')
+json = require('json')
lerror = vshard.error
--
@@ -9,10 +10,7 @@ lerror = vshard.error
ok, err = pcall(box.error, box.error.TIMEOUT)
box_error = lerror.box(err)
str = tostring(box_error)
--- Base_type appears in 2.4.1. Simple nullification of base_type
--- won't work, since order of the old fields becomes different.
-assert(str == '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}' or \
- str == '{"code":78,"base_type":"ClientError","type":"ClientError","message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}')
+util.portable_error(json.decode(str))
vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, 'uuid', 'reason')
tostring(vshard_error)
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 1/7] test: print errors in a portable way
2020-05-02 20:08 ` Vladislav Shpilevoy
@ 2020-05-04 14:26 ` Oleg Babin
0 siblings, 0 replies; 29+ messages in thread
From: Oleg Babin @ 2020-05-04 14:26 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Thanks for answers and explanation. LGTM.
On 02/05/2020 23:08, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!
>
>> On 01/05/2020 03:16, Vladislav Shpilevoy wrote:
>>> +-- Portable representation of an error, not depending on Tarantool
>>> +-- version and on any additional fields it can add. Trace is also
>>> +-- trimmed in order for the tests not to depend on line numbers of
>>> +-- the source files, which may slip into a .result file.
>>> +local function portable_error(err)
>>> + return {code = err.code, type = err.type, message = err.message}
>>> +end
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Tarantool-patches] [PATCH vshard 2/7] router: introduce discovery_mode
2020-05-01 0:16 [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 1/7] test: print errors in a portable way Vladislav Shpilevoy
@ 2020-05-01 0:16 ` Vladislav Shpilevoy
2020-05-01 16:59 ` Oleg Babin
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 3/7] test: disable router discovery for some tests Vladislav Shpilevoy
` (5 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-01 0:16 UTC (permalink / raw)
To: tarantool-patches, olegrok
Discovery disabling may be useful when there are very many routers
and buckets, and a user does not want to pay overhead of the
automatic massive discovery. It may be expensive in big clusters.
In that case users may want to turn off discovery when there is
no rebalancing, and turn it back on, when it starts, to keep the
routers up to date with _bucket changes.
Part of #210
---
test/router/router2.result | 143 +++++++++++++++++++++++++++++++++++
test/router/router2.test.lua | 51 +++++++++++++
vshard/cfg.lua | 10 +++
vshard/router/init.lua | 27 ++++++-
4 files changed, 227 insertions(+), 4 deletions(-)
create mode 100644 test/router/router2.result
create mode 100644 test/router/router2.test.lua
diff --git a/test/router/router2.result b/test/router/router2.result
new file mode 100644
index 0000000..556f749
--- /dev/null
+++ b/test/router/router2.result
@@ -0,0 +1,143 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+REPLICASET_1 = { 'storage_1_a', 'storage_1_b' }
+ | ---
+ | ...
+REPLICASET_2 = { 'storage_2_a', 'storage_2_b' }
+ | ---
+ | ...
+test_run:create_cluster(REPLICASET_1, 'router')
+ | ---
+ | ...
+test_run:create_cluster(REPLICASET_2, 'router')
+ | ---
+ | ...
+util = require('util')
+ | ---
+ | ...
+util.wait_master(test_run, REPLICASET_1, 'storage_1_a')
+ | ---
+ | ...
+util.wait_master(test_run, REPLICASET_2, 'storage_2_a')
+ | ---
+ | ...
+util.map_evals(test_run, {REPLICASET_1, REPLICASET_2}, 'bootstrap_storage(\'memtx\')')
+ | ---
+ | ...
+util.push_rs_filters(test_run)
+ | ---
+ | ...
+_ = test_run:cmd("create server router_1 with script='router/router_1.lua'")
+ | ---
+ | ...
+_ = test_run:cmd("start server router_1")
+ | ---
+ | ...
+
+_ = test_run:switch("router_1")
+ | ---
+ | ...
+util = require('util')
+ | ---
+ | ...
+
+-- gh-210: router should provide API to enable/disable discovery,
+-- since it is a too expensive thing in big clusters to be not
+-- stoppable/controllable.
+
+f1 = vshard.router.static.discovery_fiber
+ | ---
+ | ...
+cfg.discovery_mode = 'off'
+ | ---
+ | ...
+vshard.router.cfg(cfg)
+ | ---
+ | ...
+vshard.router.static.discovery_fiber
+ | ---
+ | - null
+ | ...
+f2 = vshard.router.static.discovery_fiber
+ | ---
+ | ...
+
+cfg.discovery_mode = 'on'
+ | ---
+ | ...
+vshard.router.cfg(cfg)
+ | ---
+ | ...
+f3 = vshard.router.static.discovery_fiber
+ | ---
+ | ...
+vshard.router.static.discovery_fiber:status()
+ | ---
+ | - suspended
+ | ...
+
+cfg.discovery_mode = nil
+ | ---
+ | ...
+vshard.router.cfg(cfg)
+ | ---
+ | ...
+f4 = vshard.router.static.discovery_fiber
+ | ---
+ | ...
+vshard.router.static.discovery_fiber:status()
+ | ---
+ | - suspended
+ | ...
+
+vshard.router.discovery_set('off')
+ | ---
+ | ...
+f5 = vshard.router.static.discovery_fiber
+ | ---
+ | ...
+vshard.router.static.discovery_fiber
+ | ---
+ | - null
+ | ...
+vshard.router.discovery_set('on')
+ | ---
+ | ...
+f6 = vshard.router.static.discovery_fiber
+ | ---
+ | ...
+vshard.router.static.discovery_fiber:status()
+ | ---
+ | - suspended
+ | ...
+
+f1:status(), f2, f3:status(), f4:status(), f5, f6:status()
+ | ---
+ | - dead
+ | - null
+ | - dead
+ | - dead
+ | - null
+ | - suspended
+ | ...
+
+_ = test_run:switch("default")
+ | ---
+ | ...
+_ = test_run:cmd("stop server router_1")
+ | ---
+ | ...
+_ = test_run:cmd("cleanup server router_1")
+ | ---
+ | ...
+test_run:drop_cluster(REPLICASET_1)
+ | ---
+ | ...
+test_run:drop_cluster(REPLICASET_2)
+ | ---
+ | ...
+_ = test_run:cmd('clear filter')
+ | ---
+ | ...
diff --git a/test/router/router2.test.lua b/test/router/router2.test.lua
new file mode 100644
index 0000000..33f4d3e
--- /dev/null
+++ b/test/router/router2.test.lua
@@ -0,0 +1,51 @@
+test_run = require('test_run').new()
+REPLICASET_1 = { 'storage_1_a', 'storage_1_b' }
+REPLICASET_2 = { 'storage_2_a', 'storage_2_b' }
+test_run:create_cluster(REPLICASET_1, 'router')
+test_run:create_cluster(REPLICASET_2, 'router')
+util = require('util')
+util.wait_master(test_run, REPLICASET_1, 'storage_1_a')
+util.wait_master(test_run, REPLICASET_2, 'storage_2_a')
+util.map_evals(test_run, {REPLICASET_1, REPLICASET_2}, 'bootstrap_storage(\'memtx\')')
+util.push_rs_filters(test_run)
+_ = test_run:cmd("create server router_1 with script='router/router_1.lua'")
+_ = test_run:cmd("start server router_1")
+
+_ = test_run:switch("router_1")
+util = require('util')
+
+-- gh-210: router should provide API to enable/disable discovery,
+-- since it is a too expensive thing in big clusters to be not
+-- stoppable/controllable.
+
+f1 = vshard.router.static.discovery_fiber
+cfg.discovery_mode = 'off'
+vshard.router.cfg(cfg)
+vshard.router.static.discovery_fiber
+f2 = vshard.router.static.discovery_fiber
+
+cfg.discovery_mode = 'on'
+vshard.router.cfg(cfg)
+f3 = vshard.router.static.discovery_fiber
+vshard.router.static.discovery_fiber:status()
+
+cfg.discovery_mode = nil
+vshard.router.cfg(cfg)
+f4 = vshard.router.static.discovery_fiber
+vshard.router.static.discovery_fiber:status()
+
+vshard.router.discovery_set('off')
+f5 = vshard.router.static.discovery_fiber
+vshard.router.static.discovery_fiber
+vshard.router.discovery_set('on')
+f6 = vshard.router.static.discovery_fiber
+vshard.router.static.discovery_fiber:status()
+
+f1:status(), f2, f3:status(), f4:status(), f5, f6:status()
+
+_ = test_run:switch("default")
+_ = test_run:cmd("stop server router_1")
+_ = test_run:cmd("cleanup server router_1")
+test_run:drop_cluster(REPLICASET_1)
+test_run:drop_cluster(REPLICASET_2)
+_ = test_run:cmd('clear filter')
diff --git a/vshard/cfg.lua b/vshard/cfg.lua
index 0ce7b34..8a3e812 100644
--- a/vshard/cfg.lua
+++ b/vshard/cfg.lua
@@ -151,6 +151,12 @@ local function cfg_check_weights(weights)
end
end
+local function check_discovery_mode(value)
+ if value ~= 'on' and value ~= 'off' then
+ error("Expected 'on' or 'off' for discovery_mode")
+ end
+end
+
local function check_sharding(sharding)
local uuids = {}
local uris = {}
@@ -255,6 +261,10 @@ local cfg_template = {
type = 'positive number', name = 'Failover ping timeout',
is_optional = true, default = consts.DEFAULT_FAILOVER_PING_TIMEOUT
},
+ discovery_mode = {
+ type = 'string', name = 'Discovery mode: on, off',
+ is_optional = true, default = 'on', check = check_discovery_mode
+ },
}
--
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index e430d92..ebd8356 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -64,6 +64,9 @@ local ROUTER_TEMPLATE = {
failover_fiber = nil,
-- Fiber to discovery buckets in background.
discovery_fiber = nil,
+ -- How discovery works. On - work infinitely. Off - no
+ -- discovery.
+ discovery_mode = nil,
-- Bucket count stored on all replicasets.
total_bucket_count = 0,
-- Boolean lua_gc state (create periodic gc task).
@@ -232,6 +235,7 @@ if util.version_is_at_least(1, 10, 0) then
--
discovery_f = function(router)
local module_version = M.module_version
+ assert(router.discovery_mode == 'on')
while module_version == M.module_version do
while not next(router.replicasets) do
lfiber.sleep(consts.DISCOVERY_INTERVAL)
@@ -329,6 +333,23 @@ local function discovery_wakeup(router)
end
end
+local function discovery_set(router, new_mode)
+ local current_mode = router.discovery_mode
+ if current_mode == new_mode then
+ return
+ end
+ router.discovery_mode = new_mode
+ if router.discovery_fiber ~= nil then
+ pcall(router.discovery_fiber.cancel, router.discovery_fiber)
+ end
+ if new_mode == 'off' then
+ router.discovery_fiber = nil
+ return
+ end
+ router.discovery_fiber = util.reloadable_fiber_create(
+ 'vshard.discovery.' .. router.name, M, 'discovery_f', router)
+end
+
--------------------------------------------------------------------------------
-- API
--------------------------------------------------------------------------------
@@ -792,10 +813,7 @@ local function router_cfg(router, cfg, is_reload)
router.failover_fiber = util.reloadable_fiber_create(
'vshard.failover.' .. router.name, M, 'failover_f', router)
end
- if router.discovery_fiber == nil then
- router.discovery_fiber = util.reloadable_fiber_create(
- 'vshard.discovery.' .. router.name, M, 'discovery_f', router)
- end
+ discovery_set(router, vshard_cfg.discovery_mode)
end
--------------------------------------------------------------------------------
@@ -1154,6 +1172,7 @@ local router_mt = {
bootstrap = cluster_bootstrap;
bucket_discovery = bucket_discovery;
discovery_wakeup = discovery_wakeup;
+ discovery_set = discovery_set,
}
}
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 2/7] router: introduce discovery_mode
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 2/7] router: introduce discovery_mode Vladislav Shpilevoy
@ 2020-05-01 16:59 ` Oleg Babin
0 siblings, 0 replies; 29+ messages in thread
From: Oleg Babin @ 2020-05-01 16:59 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Thanks for the patch! LGTM.
On 01/05/2020 03:16, Vladislav Shpilevoy wrote:
> Discovery disabling may be useful when there are very many routers
> and buckets, and a user does not want to pay overhead of the
> automatic massive discovery. It may be expensive in big clusters.
>
> In that case users may want to turn off discovery when there is
> no rebalancing, and turn it back on, when it starts, to keep the
> routers up to date with _bucket changes.
>
> Part of #210
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Tarantool-patches] [PATCH vshard 3/7] test: disable router discovery for some tests
2020-05-01 0:16 [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 1/7] test: print errors in a portable way Vladislav Shpilevoy
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 2/7] router: introduce discovery_mode Vladislav Shpilevoy
@ 2020-05-01 0:16 ` Vladislav Shpilevoy
2020-05-01 17:00 ` Oleg Babin
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 4/7] test: clear route map, respecting statistics Vladislav Shpilevoy
` (4 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-01 0:16 UTC (permalink / raw)
To: tarantool-patches, olegrok
There are tests, which are not related to discovery at all, and
yet they suffer from unrelated changes, when something is modified
in the discovery algorithm. For example, it highly affects the
test about request retries and automatic timeouts.
Now the discovery is disabled for such tests, since it becomes
easily possible to do with the new router's configuration option.
---
example/router.lua | 3 +++
test/router/exponential_timeout.result | 12 +++++++-----
test/router/exponential_timeout.test.lua | 4 +++-
test/router/retry_reads.result | 6 ++++--
test/router/retry_reads.test.lua | 4 +++-
test/router/router.result | 9 ++++++++-
test/router/router.test.lua | 5 ++++-
7 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/example/router.lua b/example/router.lua
index 6d0b45e..23f6c65 100755
--- a/example/router.lua
+++ b/example/router.lua
@@ -14,6 +14,9 @@ replicasets = {'cbf06940-0790-498b-948d-042b62cf3d29',
-- Call a configuration provider
cfg = require('localcfg')
+if arg[1] == 'discovery_disable' then
+ cfg.discovery_mode = 'off'
+end
cfg.listen = 3300
-- Start the database with sharding
diff --git a/test/router/exponential_timeout.result b/test/router/exponential_timeout.result
index f579382..252b816 100644
--- a/test/router/exponential_timeout.result
+++ b/test/router/exponential_timeout.result
@@ -28,7 +28,9 @@ util.map_evals(test_run, {REPLICASET_1, REPLICASET_2}, 'bootstrap_storage(\'memt
_ = test_run:cmd("create server router_1 with script='router/router_1.lua'")
---
...
-_ = test_run:cmd("start server router_1")
+-- Discovery algorithm changes sometimes and should not affect the
+-- exponential timeout test.
+_ = test_run:cmd("start server router_1 with args='discovery_disable'")
---
...
_ = test_run:switch('router_1')
@@ -49,13 +51,13 @@ util.collect_timeouts(rs1)
ok: 0
timeout: 0.5
- fail: 0
- ok: 1
+ ok: 0
timeout: 0.5
...
util.collect_timeouts(rs2)
---
- - fail: 0
- ok: 1
+ ok: 0
timeout: 0.5
- fail: 0
ok: 0
@@ -74,7 +76,7 @@ util.collect_timeouts(rs1)
ok: 0
timeout: 0.5
- fail: 0
- ok: 9
+ ok: 8
timeout: 0.5
...
_ = rs1:callrw('echo')
@@ -86,7 +88,7 @@ util.collect_timeouts(rs1)
ok: 0
timeout: 0.5
- fail: 0
- ok: 1
+ ok: 9
timeout: 0.5
...
--
diff --git a/test/router/exponential_timeout.test.lua b/test/router/exponential_timeout.test.lua
index 75b0028..881b9a7 100644
--- a/test/router/exponential_timeout.test.lua
+++ b/test/router/exponential_timeout.test.lua
@@ -10,7 +10,9 @@ util.wait_master(test_run, REPLICASET_1, 'storage_1_a')
util.wait_master(test_run, REPLICASET_2, 'storage_2_a')
util.map_evals(test_run, {REPLICASET_1, REPLICASET_2}, 'bootstrap_storage(\'memtx\')')
_ = test_run:cmd("create server router_1 with script='router/router_1.lua'")
-_ = test_run:cmd("start server router_1")
+-- Discovery algorithm changes sometimes and should not affect the
+-- exponential timeout test.
+_ = test_run:cmd("start server router_1 with args='discovery_disable'")
_ = test_run:switch('router_1')
util = require('util')
diff --git a/test/router/retry_reads.result b/test/router/retry_reads.result
index ae60267..fa38541 100644
--- a/test/router/retry_reads.result
+++ b/test/router/retry_reads.result
@@ -28,7 +28,9 @@ util.map_evals(test_run, {REPLICASET_1, REPLICASET_2}, 'bootstrap_storage(\'memt
_ = test_run:cmd("create server router_1 with script='router/router_1.lua'")
---
...
-_ = test_run:cmd("start server router_1")
+-- Discovery algorithm changes sometimes and should not affect the
+-- read retry decisions.
+_ = test_run:cmd("start server router_1 with args='discovery_disable'")
---
...
_ = test_run:switch('router_1')
@@ -53,7 +55,7 @@ util.collect_timeouts(rs1)
ok: 0
timeout: 0.5
- fail: 0
- ok: 1
+ ok: 0
timeout: 0.5
...
_ = rs1:callro('sleep', {min_timeout + 0.5}, {timeout = min_timeout})
diff --git a/test/router/retry_reads.test.lua b/test/router/retry_reads.test.lua
index c1fb689..390436a 100644
--- a/test/router/retry_reads.test.lua
+++ b/test/router/retry_reads.test.lua
@@ -10,7 +10,9 @@ util.wait_master(test_run, REPLICASET_1, 'storage_1_a')
util.wait_master(test_run, REPLICASET_2, 'storage_2_a')
util.map_evals(test_run, {REPLICASET_1, REPLICASET_2}, 'bootstrap_storage(\'memtx\')')
_ = test_run:cmd("create server router_1 with script='router/router_1.lua'")
-_ = test_run:cmd("start server router_1")
+-- Discovery algorithm changes sometimes and should not affect the
+-- read retry decisions.
+_ = test_run:cmd("start server router_1 with args='discovery_disable'")
_ = test_run:switch('router_1')
util = require('util')
diff --git a/test/router/router.result b/test/router/router.result
index d85f718..267a3e9 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -31,7 +31,8 @@ util.push_rs_filters(test_run)
_ = test_run:cmd("create server router_1 with script='router/router_1.lua'")
---
...
-_ = test_run:cmd("start server router_1")
+-- Discovery should not interfere in some first tests.
+_ = test_run:cmd("start server router_1 with args='discovery_disable'")
---
...
_ = test_run:switch("router_1")
@@ -697,6 +698,12 @@ vshard.router.call(bucket_id, 'write', 'vshard.storage.bucket_recv', {new_bid, '
--
-- Monitoring
--
+cfg.discovery_mode = 'on'
+---
+...
+vshard.router.discovery_set('on')
+---
+...
-- All is ok, when all servers are up.
-- gh-103: show bucket info for each replicaset.
info = vshard.router.info()
diff --git a/test/router/router.test.lua b/test/router/router.test.lua
index abc1a3c..c36026d 100644
--- a/test/router/router.test.lua
+++ b/test/router/router.test.lua
@@ -9,7 +9,8 @@ util.wait_master(test_run, REPLICASET_2, 'storage_2_a')
util.map_evals(test_run, {REPLICASET_1, REPLICASET_2}, 'bootstrap_storage(\'memtx\')')
util.push_rs_filters(test_run)
_ = test_run:cmd("create server router_1 with script='router/router_1.lua'")
-_ = test_run:cmd("start server router_1")
+-- Discovery should not interfere in some first tests.
+_ = test_run:cmd("start server router_1 with args='discovery_disable'")
_ = test_run:switch("router_1")
-- gh-46: Ensure a cfg is not destroyed after router.cfg().
@@ -242,6 +243,8 @@ vshard.router.call(bucket_id, 'write', 'vshard.storage.bucket_recv', {new_bid, '
-- Monitoring
--
+cfg.discovery_mode = 'on'
+vshard.router.discovery_set('on')
-- All is ok, when all servers are up.
-- gh-103: show bucket info for each replicaset.
info = vshard.router.info()
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 3/7] test: disable router discovery for some tests
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 3/7] test: disable router discovery for some tests Vladislav Shpilevoy
@ 2020-05-01 17:00 ` Oleg Babin
2020-05-02 20:09 ` Vladislav Shpilevoy
0 siblings, 1 reply; 29+ messages in thread
From: Oleg Babin @ 2020-05-01 17:00 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Hi. Thanks for the patch. LGTM. But I have a question.
On 01/05/2020 03:16, Vladislav Shpilevoy wrote:
> There are tests, which are not related to discovery at all, and
> yet they suffer from unrelated changes, when something is modified
> in the discovery algorithm. For example, it highly affects the
> test about request retries and automatic timeouts.
>
> Now the discovery is disabled for such tests, since it becomes
> easily possible to do with the new router's configuration option.
> ---
> -_ = test_run:cmd("start server router_1")
> +-- Discovery algorithm changes sometimes and should not affect the
> +-- exponential timeout test.
> +_ = test_run:cmd("start server router_1 with args='discovery_disable'")
> ---
> ...
> _ = test_run:switch('router_1')
> @@ -49,13 +51,13 @@ util.collect_timeouts(rs1)
> ok: 0
> timeout: 0.5
> - fail: 0
> - ok: 1
> + ok: 0
> timeout: 0.5
> ...
> util.collect_timeouts(rs2)
> ---
> - - fail: 0
> - ok: 1
> + ok: 0
> timeout: 0.5
> - fail: 0
> ok: 0
> @@ -74,7 +76,7 @@ util.collect_timeouts(rs1)
> ok: 0
> timeout: 0.5
> - fail: 0
> - ok: 9
> + ok: 8
> timeout: 0.5
> ...
> _ = rs1:callrw('echo')
> @@ -86,7 +88,7 @@ util.collect_timeouts(rs1)
> ok: 0
> timeout: 0.5
> - fail: 0
> - ok: 1
> + ok: 9
> timeout: 0.5
> ...
Sorry for maybe quite stupid question, but why does it quite
significantly increase?
```
net_sequential_ok - count of sequential success requests to the replica
```
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 3/7] test: disable router discovery for some tests
2020-05-01 17:00 ` Oleg Babin
@ 2020-05-02 20:09 ` Vladislav Shpilevoy
2020-05-04 14:26 ` Oleg Babin
0 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-02 20:09 UTC (permalink / raw)
To: Oleg Babin, tarantool-patches
Thanks for the review!
>> @@ -49,13 +51,13 @@ util.collect_timeouts(rs1)
>> ok: 0
>> timeout: 0.5
>> - fail: 0
>> - ok: 1
>> + ok: 0
>> timeout: 0.5
>> ...
>> util.collect_timeouts(rs2)
>> ---
>> - - fail: 0
>> - ok: 1
>> + ok: 0
>> timeout: 0.5
>> - fail: 0
>> ok: 0
>> @@ -74,7 +76,7 @@ util.collect_timeouts(rs1)
>> ok: 0
>> timeout: 0.5
>> - fail: 0
>> - ok: 9
>> + ok: 8
>> timeout: 0.5
>> ...
>> _ = rs1:callrw('echo')
>> @@ -86,7 +88,7 @@ util.collect_timeouts(rs1)
>> ok: 0
>> timeout: 0.5
>> - fail: 0
>> - ok: 1
>> + ok: 9
>> timeout: 0.5
>> ...
>
> Sorry for maybe quite stupid question, but why does it quite significantly increase?
> ```
> net_sequential_ok - count of sequential success requests to the replica
> ```
This is because the value is truncated to 1, when becomes >= 10. Since
there is no discovery, this makes number of successful requests decremented.
Before the patch value was 1. So 1 - 1 = 0, this brings the counter to its
previous round, where it ended up being 9.
After 10 the counter is truncated, because the only thing which matters -
did it manage to make 10 successful requests in a row or not. If it did,
the timeout is decreased. If it managed, then can start from 1 again.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 3/7] test: disable router discovery for some tests
2020-05-02 20:09 ` Vladislav Shpilevoy
@ 2020-05-04 14:26 ` Oleg Babin
0 siblings, 0 replies; 29+ messages in thread
From: Oleg Babin @ 2020-05-04 14:26 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Thanks for explanation. LGTM.
On 02/05/2020 23:09, Vladislav Shpilevoy wrote:
> Thanks for the review!
>
>>> @@ -49,13 +51,13 @@ util.collect_timeouts(rs1)
>>> ok: 0
>>> timeout: 0.5
>>> - fail: 0
>>> - ok: 1
>>> + ok: 0
>>> timeout: 0.5
>>> ...
>>> util.collect_timeouts(rs2)
>>> ---
>>> - - fail: 0
>>> - ok: 1
>>> + ok: 0
>>> timeout: 0.5
>>> - fail: 0
>>> ok: 0
>>> @@ -74,7 +76,7 @@ util.collect_timeouts(rs1)
>>> ok: 0
>>> timeout: 0.5
>>> - fail: 0
>>> - ok: 9
>>> + ok: 8
>>> timeout: 0.5
>>> ...
>>> _ = rs1:callrw('echo')
>>> @@ -86,7 +88,7 @@ util.collect_timeouts(rs1)
>>> ok: 0
>>> timeout: 0.5
>>> - fail: 0
>>> - ok: 1
>>> + ok: 9
>>> timeout: 0.5
>>> ...
>>
>> Sorry for maybe quite stupid question, but why does it quite significantly increase?
>> ```
>> net_sequential_ok - count of sequential success requests to the replica
>> ```
>
> This is because the value is truncated to 1, when becomes >= 10. Since
> there is no discovery, this makes number of successful requests decremented.
> Before the patch value was 1. So 1 - 1 = 0, this brings the counter to its
> previous round, where it ended up being 9.
>
> After 10 the counter is truncated, because the only thing which matters -
> did it manage to make 10 successful requests in a row or not. If it did,
> the timeout is decreased. If it managed, then can start from 1 again.
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Tarantool-patches] [PATCH vshard 4/7] test: clear route map, respecting statistics
2020-05-01 0:16 [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy
` (2 preceding siblings ...)
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 3/7] test: disable router discovery for some tests Vladislav Shpilevoy
@ 2020-05-01 0:16 ` Vladislav Shpilevoy
2020-05-01 17:00 ` Oleg Babin
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 5/7] router: keep known bucket count stat up to date Vladislav Shpilevoy
` (3 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-01 0:16 UTC (permalink / raw)
To: tarantool-patches, olegrok
Router's test sometimes need to wipe the route map. Simple reset
it to {} may produce unexpected results, because route map is not
just a table. It is also statistics in replicaset objects.
Inconsistent statistics may lead to failing tests in surprising
places. That becomes even more true with forthcoming patches,
which rework the statistics a little bit so it actually affects
something inside the router.
Part of #210
---
test/router/router.result | 12 ++++--------
test/router/router.test.lua | 10 ++++------
vshard/router/init.lua | 9 +++++++++
3 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/test/router/router.result b/test/router/router.result
index 267a3e9..df7be4a 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -175,7 +175,7 @@ vshard.router.bootstrap()
--
-- gh-108: negative bucket count on discovery.
--
-vshard.router.static.route_map = {}
+vshard.router.static:_route_map_clear()
---
...
rets = {}
@@ -985,10 +985,8 @@ _ = test_run:cmd("setopt delimiter ';'")
---
...
for i = 1, 100 do
- local rs = vshard.router.static.route_map[i]
- assert(rs)
- rs.bucket_count = rs.bucket_count - 1
- vshard.router.static.route_map[i] = nil
+ assert(vshard.router.static.route_map[i])
+ vshard.router.static:_bucket_reset(i)
end;
---
...
@@ -1243,9 +1241,7 @@ end;
vshard.router.cfg(cfg);
---
...
-vshard.router.static.route_map = {};
----
-...
+vshard.router.static:_route_map_clear()
vshard.router.internal.errinj.ERRINJ_LONG_DISCOVERY = false;
---
...
diff --git a/test/router/router.test.lua b/test/router/router.test.lua
index c36026d..97dce49 100644
--- a/test/router/router.test.lua
+++ b/test/router/router.test.lua
@@ -73,7 +73,7 @@ vshard.router.bootstrap()
--
-- gh-108: negative bucket count on discovery.
--
-vshard.router.static.route_map = {}
+vshard.router.static:_route_map_clear()
rets = {}
function do_echo() table.insert(rets, vshard.router.callro(1, 'echo', {1})) end
f1 = fiber.create(do_echo) f2 = fiber.create(do_echo)
@@ -331,10 +331,8 @@ _ = test_run:switch('router_1')
--
_ = test_run:cmd("setopt delimiter ';'")
for i = 1, 100 do
- local rs = vshard.router.static.route_map[i]
- assert(rs)
- rs.bucket_count = rs.bucket_count - 1
- vshard.router.static.route_map[i] = nil
+ assert(vshard.router.static.route_map[i])
+ vshard.router.static:_bucket_reset(i)
end;
_ = test_run:cmd("setopt delimiter ''");
calculate_known_buckets()
@@ -453,7 +451,7 @@ while vshard.router.internal.errinj.ERRINJ_LONG_DISCOVERY ~= 'waiting' do
fiber.sleep(0.02)
end;
vshard.router.cfg(cfg);
-vshard.router.static.route_map = {};
+vshard.router.static:_route_map_clear()
vshard.router.internal.errinj.ERRINJ_LONG_DISCOVERY = false;
-- Do discovery iteration. Upload buckets from the
-- first replicaset.
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index ebd8356..43b2ef7 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -112,6 +112,13 @@ local function bucket_reset(router, bucket_id)
router.route_map[bucket_id] = nil
end
+local function route_map_clear(router)
+ router.route_map = {}
+ for _, rs in pairs(router.replicasets) do
+ rs.bucket_count = 0
+ end
+end
+
--
-- Increase/decrease number of routers which require to collect
-- a lua garbage and change state of the `lua_gc` fiber.
@@ -1173,6 +1180,8 @@ local router_mt = {
bucket_discovery = bucket_discovery;
discovery_wakeup = discovery_wakeup;
discovery_set = discovery_set,
+ _route_map_clear = route_map_clear,
+ _bucket_reset = bucket_reset,
}
}
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 4/7] test: clear route map, respecting statistics
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 4/7] test: clear route map, respecting statistics Vladislav Shpilevoy
@ 2020-05-01 17:00 ` Oleg Babin
0 siblings, 0 replies; 29+ messages in thread
From: Oleg Babin @ 2020-05-01 17:00 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Hi! Thanks for the patch. LGTM.
On 01/05/2020 03:16, Vladislav Shpilevoy wrote:
> Router's test sometimes need to wipe the route map. Simple reset
> it to {} may produce unexpected results, because route map is not
> just a table. It is also statistics in replicaset objects.
>
> Inconsistent statistics may lead to failing tests in surprising
> places. That becomes even more true with forthcoming patches,
> which rework the statistics a little bit so it actually affects
> something inside the router.
>
> Part of #210
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Tarantool-patches] [PATCH vshard 5/7] router: keep known bucket count stat up to date
2020-05-01 0:16 [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy
` (3 preceding siblings ...)
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 4/7] test: clear route map, respecting statistics Vladislav Shpilevoy
@ 2020-05-01 0:16 ` Vladislav Shpilevoy
2020-05-01 17:01 ` Oleg Babin
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster Vladislav Shpilevoy
` (2 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-01 0:16 UTC (permalink / raw)
To: tarantool-patches, olegrok
Known bucket count was calculated on demand when router.info() was
called. Now it is going to be needed for advanced discovery. The
optimization will be that if known bucket count is equal to total
bucket count, the discovery enters 'idle' mode, when it works much
less aggressive, therefore reducing load on the cluster. Which can
be quite big when bucket count is huge.
Part of #210
---
vshard/router/init.lua | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index 43b2ef7..6d88153 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -69,6 +69,7 @@ local ROUTER_TEMPLATE = {
discovery_mode = nil,
-- Bucket count stored on all replicasets.
total_bucket_count = 0,
+ known_bucket_count = 0,
-- Boolean lua_gc state (create periodic gc task).
collect_lua_garbage = nil,
-- Timeout after which a ping is considered to be
@@ -96,6 +97,8 @@ local function bucket_set(router, bucket_id, rs_uuid)
if old_replicaset ~= replicaset then
if old_replicaset then
old_replicaset.bucket_count = old_replicaset.bucket_count - 1
+ else
+ router.known_bucket_count = router.known_bucket_count + 1
end
replicaset.bucket_count = replicaset.bucket_count + 1
end
@@ -108,12 +111,14 @@ local function bucket_reset(router, bucket_id)
local replicaset = router.route_map[bucket_id]
if replicaset then
replicaset.bucket_count = replicaset.bucket_count - 1
+ router.known_bucket_count = router.known_bucket_count - 1
end
router.route_map[bucket_id] = nil
end
local function route_map_clear(router)
router.route_map = {}
+ router.known_bucket_count = 0
for _, rs in pairs(router.replicasets) do
rs.bucket_count = 0
end
@@ -217,6 +222,8 @@ local function discovery_handle_buckets(router, replicaset, buckets)
affected[old_rs] = bc
end
old_rs.bucket_count = bc - 1
+ else
+ router.known_bucket_count = router.known_bucket_count + 1
end
router.route_map[bucket_id] = replicaset
end
@@ -939,7 +946,6 @@ local function router_info(router)
status = consts.STATUS.GREEN,
}
local bucket_info = state.bucket
- local known_bucket_count = 0
for rs_uuid, replicaset in pairs(router.replicasets) do
-- Replicaset info parameters:
-- * master instance info;
@@ -1007,7 +1013,6 @@ local function router_info(router)
-- available for any requests;
-- * unknown: how many buckets are unknown - a router
-- doesn't know their replicasets.
- known_bucket_count = known_bucket_count + replicaset.bucket_count
if rs_info.master.status ~= 'available' then
if rs_info.replica.status ~= 'available' then
rs_info.bucket.unreachable = replicaset.bucket_count
@@ -1028,7 +1033,7 @@ local function router_info(router)
-- If a bucket is unreachable, then replicaset is
-- unreachable too and color already is red.
end
- bucket_info.unknown = router.total_bucket_count - known_bucket_count
+ bucket_info.unknown = router.total_bucket_count - router.known_bucket_count
if bucket_info.unknown > 0 then
state.status = math.max(state.status, consts.STATUS.YELLOW)
table.insert(state.alerts, lerror.alert(lerror.code.UNKNOWN_BUCKETS,
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 29+ messages in thread* [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster
2020-05-01 0:16 [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy
` (4 preceding siblings ...)
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 5/7] router: keep known bucket count stat up to date Vladislav Shpilevoy
@ 2020-05-01 0:16 ` Vladislav Shpilevoy
2020-05-01 17:01 ` Oleg Babin
2020-05-07 22:45 ` Konstantin Osipov
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 7/7] router: introduce discovery mode 'once' Vladislav Shpilevoy
2020-05-06 20:54 ` [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy
7 siblings, 2 replies; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-01 0:16 UTC (permalink / raw)
To: tarantool-patches, olegrok
Router does discovery once per 10 seconds. Discovery sends a
request to each replicaset to download all pinned and active
buckets from there. When there are millions of buckets, that
becomes a long operation taking seconds, during which the storage
is unresponsive.
The patch makes discovery work step by step, downloading not more
than 1000 buckets at a time. That gives the storage time to
process other requests.
Moreover, discovery now has some kind of 'state'. For each
replicaset it keeps an iterator which is moved by 1k buckets on
every successfully discovered bucket batch. It means, that if on a
replicaset with 1 000 000 buckets discovery fails after 999 999
buckets are already discovered, it won't start from 0. It will
retry from the old position.
However, still there is space for improvement. Discovery could
avoid downloading anything after all is downloaded, if it could
somehow see, if bucket space is not changed. Unfortunately it is
not so easy, since bucket generation (version of _bucket space)
is not persisted. So after instance restart it is always equal to
bucket count.
Part of #210
---
test/router/reload.result | 5 +-
test/router/reload.test.lua | 3 +-
test/router/router.result | 4 +-
test/router/router.test.lua | 2 +-
test/router/wrong_config.result | 5 +-
test/router/wrong_config.test.lua | 5 +-
vshard/consts.lua | 5 +-
vshard/router/init.lua | 145 +++++++++++++++++++++++-------
vshard/storage/init.lua | 39 +++++++-
9 files changed, 170 insertions(+), 43 deletions(-)
diff --git a/test/router/reload.result b/test/router/reload.result
index 3ba900a..8fe99ba 100644
--- a/test/router/reload.result
+++ b/test/router/reload.result
@@ -44,7 +44,10 @@ vshard.router.bootstrap()
while test_run:grep_log('router_1', 'All replicas are ok') == nil do fiber.sleep(0.1) end
---
...
-while test_run:grep_log('router_1', 'buckets: was 0, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
+while test_run:grep_log('router_1', 'buckets: was 0, became 1000') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
+---
+...
+while test_run:grep_log('router_1', 'buckets: was 1000, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
---
...
--
diff --git a/test/router/reload.test.lua b/test/router/reload.test.lua
index 5ed5690..abcbc09 100644
--- a/test/router/reload.test.lua
+++ b/test/router/reload.test.lua
@@ -15,7 +15,8 @@ fiber = require('fiber')
vshard.router.bootstrap()
while test_run:grep_log('router_1', 'All replicas are ok') == nil do fiber.sleep(0.1) end
-while test_run:grep_log('router_1', 'buckets: was 0, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
+while test_run:grep_log('router_1', 'buckets: was 0, became 1000') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
+while test_run:grep_log('router_1', 'buckets: was 1000, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
--
-- Gh-72: allow reload. Test simple reload, error during
diff --git a/test/router/router.result b/test/router/router.result
index df7be4a..b2efd6d 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -943,9 +943,9 @@ calculate_known_buckets()
---
- 3000
...
-test_run:grep_log('router_1', 'was 1, became 1500')
+test_run:grep_log('router_1', 'was 1, became 1000')
---
-- was 1, became 1500
+- was 1, became 1000
...
info = vshard.router.info()
---
diff --git a/test/router/router.test.lua b/test/router/router.test.lua
index 97dce49..154310b 100644
--- a/test/router/router.test.lua
+++ b/test/router/router.test.lua
@@ -316,7 +316,7 @@ vshard.storage.bucket_pin(first_active)
_ = test_run:switch('router_1')
wait_discovery()
calculate_known_buckets()
-test_run:grep_log('router_1', 'was 1, became 1500')
+test_run:grep_log('router_1', 'was 1, became 1000')
info = vshard.router.info()
info.bucket
info.alerts
diff --git a/test/router/wrong_config.result b/test/router/wrong_config.result
index 56db9e8..92353c3 100644
--- a/test/router/wrong_config.result
+++ b/test/router/wrong_config.result
@@ -45,7 +45,10 @@ cfg.bucket_count = 1000
r = vshard.router.new('gh-179', cfg)
---
...
-while type(r:info().bucket.unknown) == 'number' and r:info().bucket.unknown > 0 do r:discovery_wakeup() fiber.sleep(0.1) end
+while r:info().bucket.available_rw ~= 3000 do \
+ r:discovery_wakeup() \
+ fiber.sleep(0.1) \
+end
---
...
i = r:info()
diff --git a/test/router/wrong_config.test.lua b/test/router/wrong_config.test.lua
index 62ef30d..174b373 100644
--- a/test/router/wrong_config.test.lua
+++ b/test/router/wrong_config.test.lua
@@ -18,7 +18,10 @@ vshard.router.bootstrap()
--
cfg.bucket_count = 1000
r = vshard.router.new('gh-179', cfg)
-while type(r:info().bucket.unknown) == 'number' and r:info().bucket.unknown > 0 do r:discovery_wakeup() fiber.sleep(0.1) end
+while r:info().bucket.available_rw ~= 3000 do \
+ r:discovery_wakeup() \
+ fiber.sleep(0.1) \
+end
i = r:info()
i.bucket
i.alerts
diff --git a/vshard/consts.lua b/vshard/consts.lua
index 5391c0f..a6a8c1b 100644
--- a/vshard/consts.lua
+++ b/vshard/consts.lua
@@ -40,5 +40,8 @@ return {
DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL = 0.5;
RECOVERY_INTERVAL = 5;
COLLECT_LUA_GARBAGE_INTERVAL = 100;
- DISCOVERY_INTERVAL = 10;
+ DISCOVERY_IDLE_INTERVAL = 10,
+ DISCOVERY_WORK_INTERVAL = 1,
+ DISCOVERY_WORK_STEP = 0.01,
+ DISCOVERY_TIMEOUT = 10,
}
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index 6d88153..26ea85b 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -250,50 +250,127 @@ if util.version_is_at_least(1, 10, 0) then
discovery_f = function(router)
local module_version = M.module_version
assert(router.discovery_mode == 'on')
+ local iterators = {}
+ local opts = {is_async = true}
+ local mode
while module_version == M.module_version do
- while not next(router.replicasets) do
- lfiber.sleep(consts.DISCOVERY_INTERVAL)
- end
- if module_version ~= M.module_version then
- return
- end
-- Just typical map reduce - send request to each
- -- replicaset in parallel, and collect responses.
- local pending = {}
- local opts = {is_async = true}
- local args = {}
- for rs_uuid, replicaset in pairs(router.replicasets) do
+ -- replicaset in parallel, and collect responses. Many
+ -- requests probably will be needed for each replicaset.
+ --
+ -- Step 1: create missing iterators, in case this is a
+ -- first discovery iteration, or some replicasets were
+ -- added after the router is started.
+ for rs_uuid in pairs(router.replicasets) do
+ local iter = iterators[rs_uuid]
+ if not iter then
+ iterators[rs_uuid] = {
+ args = {{from = 1}},
+ future = nil,
+ }
+ end
+ end
+ -- Step 2: map stage - send parallel requests for every
+ -- iterator, prune orphan iterators whose replicasets were
+ -- removed.
+ for rs_uuid, iter in pairs(iterators) do
+ local replicaset = router.replicasets[rs_uuid]
+ if not replicaset then
+ log.warn('Replicaset %s was removed during discovery', rs_uuid)
+ iterators[rs_uuid] = nil
+ goto continue
+ end
local future, err =
- replicaset:callro('vshard.storage.buckets_discovery',
- args, opts)
+ replicaset:callro('vshard.storage.buckets_discovery', iter.args,
+ opts)
if not future then
- log.warn('Error during discovery %s: %s', rs_uuid, err)
- else
- pending[rs_uuid] = future
+ log.warn('Error during discovery %s, retry will be done '..
+ 'later: %s', rs_uuid, err)
+ goto continue
+ end
+ iter.future = future
+ -- Don't spam many requests at once. Give
+ -- storages time to handle them and other
+ -- requests.
+ lfiber.sleep(consts.DISCOVERY_WORK_STEP)
+ if module_version ~= M.module_version then
+ return
end
+ ::continue::
end
-
- local deadline = lfiber.clock() + consts.DISCOVERY_INTERVAL
- for rs_uuid, p in pairs(pending) do
+ -- Step 3: reduce stage - collect responses, restart
+ -- iterators which reached the end.
+ for rs_uuid, iter in pairs(iterators) do
lfiber.yield()
- local timeout = deadline - lfiber.clock()
- local buckets, err = p:wait_result(timeout)
- while M.errinj.ERRINJ_LONG_DISCOVERY do
- M.errinj.ERRINJ_LONG_DISCOVERY = 'waiting'
- lfiber.sleep(0.01)
+ local future = iter.future
+ if not future then
+ goto continue
end
- local replicaset = router.replicasets[rs_uuid]
- if not buckets then
- p:discard()
- log.warn('Error during discovery %s: %s', rs_uuid, err)
- elseif module_version ~= M.module_version then
+ local result, err = future:wait_result(consts.DISCOVERY_TIMEOUT)
+ if module_version ~= M.module_version then
return
- elseif replicaset then
- discovery_handle_buckets(router, replicaset, buckets[1])
end
+ if not result then
+ future:discard()
+ log.warn('Error during discovery %s, retry will be done '..
+ 'later: %s', rs_uuid, err)
+ goto continue
+ end
+ local replicaset = router.replicasets[rs_uuid]
+ if not replicaset then
+ iterators[rs_uuid] = nil
+ log.warn('Replicaset %s was removed during discovery', rs_uuid)
+ goto continue
+ end
+ result = result[1]
+ -- Buckets are returned as plain array by storages
+ -- using old vshard version. But if .buckets is set,
+ -- this is a new storage.
+ discovery_handle_buckets(router, replicaset,
+ result.buckets or result)
+ local discovery_args = iter.args[1]
+ discovery_args.from = result.next_from
+ if not result.next_from then
+ -- Nil next_from means no more buckets to get.
+ -- Restart the iterator.
+ iterators[rs_uuid] = nil
+ end
+ ::continue::
end
-
- lfiber.sleep(deadline - lfiber.clock())
+ local unknown_bucket_count
+ repeat
+ unknown_bucket_count =
+ router.total_bucket_count - router.known_bucket_count
+ if unknown_bucket_count == 0 then
+ if mode ~= 'idle' then
+ log.info('Discovery enters idle mode, all buckets are '..
+ 'known. Discovery works with %s seconds '..
+ 'interval now', consts.DISCOVERY_IDLE_INTERVAL)
+ mode = 'idle'
+ end
+ lfiber.sleep(consts.DISCOVERY_IDLE_INTERVAL)
+ elseif not next(router.replicasets) then
+ if mode ~= 'idle' then
+ log.info('Discovery enters idle mode because '..
+ 'configuration does not have replicasets. '..
+ 'Retries will happen with %s seconds interval',
+ consts.DISCOVERY_IDLE_INTERVAL)
+ mode = 'idle'
+ end
+ lfiber.sleep(consts.DISCOVERY_IDLE_INTERVAL)
+ elseif mode ~= 'aggressive' then
+ log.info('Start aggressive discovery, %s buckets are unknown. '..
+ 'Discovery works with %s seconds interval',
+ unknown_bucket_count, consts.DISCOVERY_WORK_INTERVAL)
+ mode = 'aggressive'
+ lfiber.sleep(consts.DISCOVERY_WORK_INTERVAL)
+ break
+ end
+ while M.errinj.ERRINJ_LONG_DISCOVERY do
+ M.errinj.ERRINJ_LONG_DISCOVERY = 'waiting'
+ lfiber.sleep(0.01)
+ end
+ until next(router.replicasets)
end
end
@@ -355,9 +432,9 @@ local function discovery_set(router, new_mode)
router.discovery_mode = new_mode
if router.discovery_fiber ~= nil then
pcall(router.discovery_fiber.cancel, router.discovery_fiber)
+ router.discovery_fiber = nil
end
if new_mode == 'off' then
- router.discovery_fiber = nil
return
end
router.discovery_fiber = util.reloadable_fiber_create(
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
index 73c6740..0050b96 100644
--- a/vshard/storage/init.lua
+++ b/vshard/storage/init.lua
@@ -1110,10 +1110,47 @@ local function bucket_collect(bucket_id)
return data
end
+-- Discovery used by routers. It returns limited number of
+-- buckets to avoid stalls when _bucket is huge.
+local function buckets_discovery_extended(opts)
+ local limit = consts.BUCKET_CHUNK_SIZE
+ local buckets = table.new(limit, 0)
+ local active = consts.BUCKET.ACTIVE
+ local pinned = consts.BUCKET.PINNED
+ local next_from
+ -- No way to select by {status, id}, because there are two
+ -- statuses to select. A router would need to maintain a
+ -- separate iterator for each status it wants to get. This may
+ -- be implemented in future. But _bucket space anyway 99% of
+ -- time contains only active and pinned buckets. So there is
+ -- no big benefit in optimizing that. Perhaps a compound index
+ -- {status, id} could help too.
+ for _, bucket in box.space._bucket:pairs({opts.from},
+ {iterator = box.index.GE}) do
+ local status = bucket.status
+ if status == active or status == pinned then
+ table.insert(buckets, bucket.id)
+ end
+ limit = limit - 1
+ if limit == 0 then
+ next_from = bucket.id + 1
+ break
+ end
+ end
+ -- Buckets list can even be empty, if all buckets in the
+ -- scanned chunk are not active/pinned. But next_from still
+ -- should be returned. So as the router could request more.
+ return {buckets = buckets, next_from = next_from}
+end
+
--
-- Collect array of active bucket identifiers for discovery.
--
-local function buckets_discovery()
+local function buckets_discovery(opts)
+ if opts then
+ -- Private method. Is not documented intentionally.
+ return buckets_discovery_extended(opts)
+ end
local ret = {}
local status = box.space._bucket.index.status
for _, bucket in status:pairs({consts.BUCKET.ACTIVE}) do
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster Vladislav Shpilevoy
@ 2020-05-01 17:01 ` Oleg Babin
2020-05-02 20:12 ` Vladislav Shpilevoy
2020-05-07 22:45 ` Konstantin Osipov
1 sibling, 1 reply; 29+ messages in thread
From: Oleg Babin @ 2020-05-01 17:01 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Thanks for your patch! See my comments below.
On 01/05/2020 03:16, Vladislav Shpilevoy wrote:
> Router does discovery once per 10 seconds. Discovery sends a
> request to each replicaset to download all pinned and active
> buckets from there. When there are millions of buckets, that
> becomes a long operation taking seconds, during which the storage
> is unresponsive.
>
> The patch makes discovery work step by step, downloading not more
> than 1000 buckets at a time. That gives the storage time to
> process other requests.
>
> Moreover, discovery now has some kind of 'state'. For each
> replicaset it keeps an iterator which is moved by 1k buckets on
> every successfully discovered bucket batch. It means, that if on a
> replicaset with 1 000 000 buckets discovery fails after 999 999
> buckets are already discovered, it won't start from 0. It will
> retry from the old position.
Could you provide a test for such case?
> However, still there is space for improvement. Discovery could
> avoid downloading anything after all is downloaded, if it could
> somehow see, if bucket space is not changed. Unfortunately it is
> not so easy, since bucket generation (version of _bucket space)
> is not persisted. So after instance restart it is always equal to
> bucket count.
Is there an issue for that?
> Part of #210
> ---
> test/router/reload.result | 5 +-
> test/router/reload.test.lua | 3 +-
> test/router/router.result | 4 +-
> test/router/router.test.lua | 2 +-
> test/router/wrong_config.result | 5 +-
> test/router/wrong_config.test.lua | 5 +-
> vshard/consts.lua | 5 +-
> vshard/router/init.lua | 145 +++++++++++++++++++++++-------
> vshard/storage/init.lua | 39 +++++++-
> 9 files changed, 170 insertions(+), 43 deletions(-)
>
> diff --git a/test/router/reload.result b/test/router/reload.result
> index 3ba900a..8fe99ba 100644
> --- a/test/router/reload.result
> +++ b/test/router/reload.result
> @@ -44,7 +44,10 @@ vshard.router.bootstrap()
> while test_run:grep_log('router_1', 'All replicas are ok') == nil do fiber.sleep(0.1) end
> ---
> ...
> -while test_run:grep_log('router_1', 'buckets: was 0, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
> +while test_run:grep_log('router_1', 'buckets: was 0, became 1000') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
> +---
> +...
> +while test_run:grep_log('router_1', 'buckets: was 1000, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
> ---
> ...
> --
> diff --git a/test/router/reload.test.lua b/test/router/reload.test.lua
> index 5ed5690..abcbc09 100644
> --- a/test/router/reload.test.lua
> +++ b/test/router/reload.test.lua
> @@ -15,7 +15,8 @@ fiber = require('fiber')
> vshard.router.bootstrap()
>
> while test_run:grep_log('router_1', 'All replicas are ok') == nil do fiber.sleep(0.1) end
> -while test_run:grep_log('router_1', 'buckets: was 0, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
> +while test_run:grep_log('router_1', 'buckets: was 0, became 1000') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
> +while test_run:grep_log('router_1', 'buckets: was 1000, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
>
> --
> -- Gh-72: allow reload. Test simple reload, error during
> diff --git a/test/router/router.result b/test/router/router.result
> index df7be4a..b2efd6d 100644
> --- a/test/router/router.result
> +++ b/test/router/router.result
> @@ -943,9 +943,9 @@ calculate_known_buckets()
> ---
> - 3000
> ...
> -test_run:grep_log('router_1', 'was 1, became 1500')
> +test_run:grep_log('router_1', 'was 1, became 1000')
> ---
> -- was 1, became 1500
> +- was 1, became 1000
> ...
> info = vshard.router.info()
> ---
> diff --git a/test/router/router.test.lua b/test/router/router.test.lua
> index 97dce49..154310b 100644
> --- a/test/router/router.test.lua
> +++ b/test/router/router.test.lua
> @@ -316,7 +316,7 @@ vshard.storage.bucket_pin(first_active)
> _ = test_run:switch('router_1')
> wait_discovery()
> calculate_known_buckets()
> -test_run:grep_log('router_1', 'was 1, became 1500')
> +test_run:grep_log('router_1', 'was 1, became 1000')
> info = vshard.router.info()
> info.bucket
> info.alerts
> diff --git a/test/router/wrong_config.result b/test/router/wrong_config.result
> index 56db9e8..92353c3 100644
> --- a/test/router/wrong_config.result
> +++ b/test/router/wrong_config.result
> @@ -45,7 +45,10 @@ cfg.bucket_count = 1000
> r = vshard.router.new('gh-179', cfg)
> ---
> ...
> -while type(r:info().bucket.unknown) == 'number' and r:info().bucket.unknown > 0 do r:discovery_wakeup() fiber.sleep(0.1) end
> +while r:info().bucket.available_rw ~= 3000 do \
> + r:discovery_wakeup() \
> + fiber.sleep(0.1) \
> +end
> ---
> ...
> i = r:info()
> diff --git a/test/router/wrong_config.test.lua b/test/router/wrong_config.test.lua
> index 62ef30d..174b373 100644
> --- a/test/router/wrong_config.test.lua
> +++ b/test/router/wrong_config.test.lua
> @@ -18,7 +18,10 @@ vshard.router.bootstrap()
> --
> cfg.bucket_count = 1000
> r = vshard.router.new('gh-179', cfg)
> -while type(r:info().bucket.unknown) == 'number' and r:info().bucket.unknown > 0 do r:discovery_wakeup() fiber.sleep(0.1) end
> +while r:info().bucket.available_rw ~= 3000 do \
> + r:discovery_wakeup() \
> + fiber.sleep(0.1) \
> +end
> i = r:info()
> i.bucket
> i.alerts
> diff --git a/vshard/consts.lua b/vshard/consts.lua
> index 5391c0f..a6a8c1b 100644
> --- a/vshard/consts.lua
> +++ b/vshard/consts.lua
> @@ -40,5 +40,8 @@ return {
> DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL = 0.5;
> RECOVERY_INTERVAL = 5;
> COLLECT_LUA_GARBAGE_INTERVAL = 100;
> - DISCOVERY_INTERVAL = 10;
> + DISCOVERY_IDLE_INTERVAL = 10,
> + DISCOVERY_WORK_INTERVAL = 1,
> + DISCOVERY_WORK_STEP = 0.01,
> + DISCOVERY_TIMEOUT = 10,
> }
> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> index 6d88153..26ea85b 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -250,50 +250,127 @@ if util.version_is_at_least(1, 10, 0) then
> discovery_f = function(router)
> local module_version = M.module_version
> assert(router.discovery_mode == 'on')
> + local iterators = {}
> + local opts = {is_async = true}
> + local mode
> while module_version == M.module_version do
> - while not next(router.replicasets) do
> - lfiber.sleep(consts.DISCOVERY_INTERVAL)
> - end
> - if module_version ~= M.module_version then
> - return
> - end
> -- Just typical map reduce - send request to each
> - -- replicaset in parallel, and collect responses.
> - local pending = {}
> - local opts = {is_async = true}
> - local args = {}
> - for rs_uuid, replicaset in pairs(router.replicasets) do
> + -- replicaset in parallel, and collect responses. Many
> + -- requests probably will be needed for each replicaset.
> + --
> + -- Step 1: create missing iterators, in case this is a
> + -- first discovery iteration, or some replicasets were
> + -- added after the router is started.
> + for rs_uuid in pairs(router.replicasets) do
> + local iter = iterators[rs_uuid]
> + if not iter then
> + iterators[rs_uuid] = {
> + args = {{from = 1}},
> + future = nil,
> + }
> + end
> + end
> + -- Step 2: map stage - send parallel requests for every
> + -- iterator, prune orphan iterators whose replicasets were
> + -- removed.
> + for rs_uuid, iter in pairs(iterators) do
> + local replicaset = router.replicasets[rs_uuid]
> + if not replicaset then
> + log.warn('Replicaset %s was removed during discovery', rs_uuid)
> + iterators[rs_uuid] = nil
> + goto continue
> + end
> local future, err =
> - replicaset:callro('vshard.storage.buckets_discovery',
> - args, opts)
> + replicaset:callro('vshard.storage.buckets_discovery', iter.args,
> + opts)
> if not future then
> - log.warn('Error during discovery %s: %s', rs_uuid, err)
> - else
> - pending[rs_uuid] = future
> + log.warn('Error during discovery %s, retry will be done '..
> + 'later: %s', rs_uuid, err)
> + goto continue
> + end
> + iter.future = future
> + -- Don't spam many requests at once. Give
> + -- storages time to handle them and other
> + -- requests.
> + lfiber.sleep(consts.DISCOVERY_WORK_STEP)
> + if module_version ~= M.module_version then
> + return
> end
Is it possible to place such checks to some "common" places. At
start/end of each iteration or between steps. This looks strange and a
bit unobvous to do such checks after each timeout.
> + ::continue::
> end
> -
> - local deadline = lfiber.clock() + consts.DISCOVERY_INTERVAL
> - for rs_uuid, p in pairs(pending) do
> + -- Step 3: reduce stage - collect responses, restart
> + -- iterators which reached the end.
> + for rs_uuid, iter in pairs(iterators) do
> lfiber.yield()
> - local timeout = deadline - lfiber.clock()
> - local buckets, err = p:wait_result(timeout)
> - while M.errinj.ERRINJ_LONG_DISCOVERY do
> - M.errinj.ERRINJ_LONG_DISCOVERY = 'waiting'
> - lfiber.sleep(0.01)
> + local future = iter.future
> + if not future then
> + goto continue
> end
> - local replicaset = router.replicasets[rs_uuid]
> - if not buckets then
> - p:discard()
> - log.warn('Error during discovery %s: %s', rs_uuid, err)
> - elseif module_version ~= M.module_version then
> + local result, err = future:wait_result(consts.DISCOVERY_TIMEOUT)
> + if module_version ~= M.module_version then
Does it have sence to call "discard" before "return"?
> return
> - elseif replicaset then
> - discovery_handle_buckets(router, replicaset, buckets[1])
> end
> + if not result then
> + future:discard()
> + log.warn('Error during discovery %s, retry will be done '..
> + 'later: %s', rs_uuid, err)
> + goto continue
> + end
> + local replicaset = router.replicasets[rs_uuid]
> + if not replicaset then
> + iterators[rs_uuid] = nil
> + log.warn('Replicaset %s was removed during discovery', rs_uuid)
> + goto continue
> + end
> + result = result[1]
> + -- Buckets are returned as plain array by storages
> + -- using old vshard version. But if .buckets is set,
> + -- this is a new storage.
> + discovery_handle_buckets(router, replicaset,
> + result.buckets or result)
> + local discovery_args = iter.args[1]
> + discovery_args.from = result.next_from
> + if not result.next_from then
> + -- Nil next_from means no more buckets to get.
> + -- Restart the iterator.
> + iterators[rs_uuid] = nil
> + end
> + ::continue::
> end
> -
> - lfiber.sleep(deadline - lfiber.clock())
> + local unknown_bucket_count
> + repeat
> + unknown_bucket_count =
> + router.total_bucket_count - router.known_bucket_count
> + if unknown_bucket_count == 0 then
> + if mode ~= 'idle' then
> + log.info('Discovery enters idle mode, all buckets are '..
> + 'known. Discovery works with %s seconds '..
> + 'interval now', consts.DISCOVERY_IDLE_INTERVAL)
> + mode = 'idle'
> + end
> + lfiber.sleep(consts.DISCOVERY_IDLE_INTERVAL)
> + elseif not next(router.replicasets) then
> + if mode ~= 'idle' then
> + log.info('Discovery enters idle mode because '..
> + 'configuration does not have replicasets. '..
> + 'Retries will happen with %s seconds interval',
> + consts.DISCOVERY_IDLE_INTERVAL)
> + mode = 'idle'
> + end
> + lfiber.sleep(consts.DISCOVERY_IDLE_INTERVAL)
> + elseif mode ~= 'aggressive' then
> + log.info('Start aggressive discovery, %s buckets are unknown. '..
> + 'Discovery works with %s seconds interval',
> + unknown_bucket_count, consts.DISCOVERY_WORK_INTERVAL)
> + mode = 'aggressive'
> + lfiber.sleep(consts.DISCOVERY_WORK_INTERVAL)
> + break
> + end
> + while M.errinj.ERRINJ_LONG_DISCOVERY do
> + M.errinj.ERRINJ_LONG_DISCOVERY = 'waiting'
> + lfiber.sleep(0.01)
> + end
> + until next(router.replicasets)
> end
> end
>
> @@ -355,9 +432,9 @@ local function discovery_set(router, new_mode)
> router.discovery_mode = new_mode
> if router.discovery_fiber ~= nil then
> pcall(router.discovery_fiber.cancel, router.discovery_fiber)
> + router.discovery_fiber = nil
> end
> if new_mode == 'off' then
> - router.discovery_fiber = nil
> return
> end
> router.discovery_fiber = util.reloadable_fiber_create(
> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
> index 73c6740..0050b96 100644
> --- a/vshard/storage/init.lua
> +++ b/vshard/storage/init.lua
> @@ -1110,10 +1110,47 @@ local function bucket_collect(bucket_id)
> return data
> end
>
> +-- Discovery used by routers. It returns limited number of
> +-- buckets to avoid stalls when _bucket is huge.
> +local function buckets_discovery_extended(opts)
> + local limit = consts.BUCKET_CHUNK_SIZE
> + local buckets = table.new(limit, 0)
> + local active = consts.BUCKET.ACTIVE
> + local pinned = consts.BUCKET.PINNED
> + local next_from
> + -- No way to select by {status, id}, because there are two
> + -- statuses to select. A router would need to maintain a
> + -- separate iterator for each status it wants to get. This may
> + -- be implemented in future. But _bucket space anyway 99% of
> + -- time contains only active and pinned buckets. So there is
> + -- no big benefit in optimizing that. Perhaps a compound index
> + -- {status, id} could help too.
> + for _, bucket in box.space._bucket:pairs({opts.from},
> + {iterator = box.index.GE}) do
> + local status = bucket.status
> + if status == active or status == pinned then
> + table.insert(buckets, bucket.id)
> + end
> + limit = limit - 1
It's a bit strange after words about "99% time", I propose to move limit
decrease under if condition.
> + if limit == 0 then
> + next_from = bucket.id + 1
> + break
> + end
> + end
> + -- Buckets list can even be empty, if all buckets in the
> + -- scanned chunk are not active/pinned. But next_from still
> + -- should be returned. So as the router could request more.
> + return {buckets = buckets, next_from = next_from}
> +end
> +
> --
> -- Collect array of active bucket identifiers for discovery.
> --
> -local function buckets_discovery()
> +local function buckets_discovery(opts)
> + if opts then
> + -- Private method. Is not documented intentionally.
> + return buckets_discovery_extended(opts)
> + end
> local ret = {}
> local status = box.space._bucket.index.status
> for _, bucket in status:pairs({consts.BUCKET.ACTIVE}) do
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster
2020-05-01 17:01 ` Oleg Babin
@ 2020-05-02 20:12 ` Vladislav Shpilevoy
2020-05-04 14:26 ` Oleg Babin
0 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-02 20:12 UTC (permalink / raw)
To: Oleg Babin, tarantool-patches
Thanks for the review!
On 01/05/2020 19:01, Oleg Babin wrote:
> Thanks for your patch! See my comments below.
>
> On 01/05/2020 03:16, Vladislav Shpilevoy wrote:
>> Router does discovery once per 10 seconds. Discovery sends a
>> request to each replicaset to download all pinned and active
>> buckets from there. When there are millions of buckets, that
>> becomes a long operation taking seconds, during which the storage
>> is unresponsive.
>>
>> The patch makes discovery work step by step, downloading not more
>> than 1000 buckets at a time. That gives the storage time to
>> process other requests.
>>
>> Moreover, discovery now has some kind of 'state'. For each
>> replicaset it keeps an iterator which is moved by 1k buckets on
>> every successfully discovered bucket batch. It means, that if on a
>> replicaset with 1 000 000 buckets discovery fails after 999 999
>> buckets are already discovered, it won't start from 0. It will
>> retry from the old position.
>
> Could you provide a test for such case?
No problem.
====================
diff --git a/test/router/router2.result b/test/router/router2.result
index 556f749..0ad5a21 100644
--- a/test/router/router2.result
+++ b/test/router/router2.result
@@ -123,6 +123,66 @@ f1:status(), f2, f3:status(), f4:status(), f5, f6:status()
| - suspended
| ...
+-- Errored discovery continued successfully after errors are gone.
+vshard.router.bootstrap()
+ | ---
+ | - true
+ | ...
+vshard.router.discovery_set('off')
+ | ---
+ | ...
+vshard.router._route_map_clear()
+ | ---
+ | ...
+
+-- Discovery requests 2 and 4 will fail on storages.
+util.map_evals(test_run, {{'storage_1_a'}, {'storage_2_a'}}, \
+ 'vshard.storage.internal.errinj.ERRINJ_DISCOVERY = 4')
+ | ---
+ | ...
+
+vshard.router.info().bucket.unknown
+ | ---
+ | - 3000
+ | ...
+vshard.router.discovery_set('on')
+ | ---
+ | ...
+function continue_discovery() \
+ local res = vshard.router.info().bucket.unknown == 0 \
+ if not res then \
+ vshard.router.discovery_wakeup() \
+ end \
+ return res \
+end
+ | ---
+ | ...
+test_run:wait_cond(continue_discovery)
+ | ---
+ | - true
+ | ...
+vshard.router.info().bucket.unknown
+ | ---
+ | - 0
+ | ...
+
+-- Discovery injections should be reset meaning they were returned
+-- needed number of times.
+_ = test_run:switch('storage_1_a')
+ | ---
+ | ...
+vshard.storage.internal.errinj.ERRINJ_DISCOVERY
+ | ---
+ | - 0
+ | ...
+_ = test_run:switch('storage_2_a')
+ | ---
+ | ...
+vshard.storage.internal.errinj.ERRINJ_DISCOVERY
+ | ---
+ | - 0
+ | ...
+
_ = test_run:switch("default")
| ---
| ...
diff --git a/test/router/router2.test.lua b/test/router/router2.test.lua
index 33f4d3e..13fa73f 100644
--- a/test/router/router2.test.lua
+++ b/test/router/router2.test.lua
@@ -43,6 +43,34 @@ vshard.router.static.discovery_fiber:status()
f1:status(), f2, f3:status(), f4:status(), f5, f6:status()
+-- Errored discovery continued successfully after errors are gone.
+vshard.router.bootstrap()
+vshard.router.discovery_set('off')
+vshard.router._route_map_clear()
+
+-- Discovery requests 2 and 4 will fail on storages.
+util.map_evals(test_run, {{'storage_1_a'}, {'storage_2_a'}}, \
+ 'vshard.storage.internal.errinj.ERRINJ_DISCOVERY = 4')
+
+vshard.router.info().bucket.unknown
+vshard.router.discovery_set('on')
+function continue_discovery() \
+ local res = vshard.router.info().bucket.unknown == 0 \
+ if not res then \
+ vshard.router.discovery_wakeup() \
+ end \
+ return res \
+end
+test_run:wait_cond(continue_discovery)
+vshard.router.info().bucket.unknown
+
+-- Discovery injections should be reset meaning they were returned
+-- needed number of times.
+_ = test_run:switch('storage_1_a')
+vshard.storage.internal.errinj.ERRINJ_DISCOVERY
+_ = test_run:switch('storage_2_a')
+vshard.storage.internal.errinj.ERRINJ_DISCOVERY
+
_ = test_run:switch("default")
_ = test_run:cmd("stop server router_1")
_ = test_run:cmd("cleanup server router_1")
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
index 0050b96..c6a78fe 100644
--- a/vshard/storage/init.lua
+++ b/vshard/storage/init.lua
@@ -75,6 +75,7 @@ if not M then
ERRINJ_RECEIVE_PARTIALLY = false,
ERRINJ_NO_RECOVERY = false,
ERRINJ_UPGRADE = false,
+ ERRINJ_DISCOVERY = false,
},
-- This counter is used to restart background fibers with
-- new reloaded code.
@@ -1118,6 +1119,17 @@ local function buckets_discovery_extended(opts)
local active = consts.BUCKET.ACTIVE
local pinned = consts.BUCKET.PINNED
local next_from
+ local errcnt = M.errinj.ERRINJ_DISCOVERY
+ if errcnt then
+ if errcnt > 0 then
+ M.errinj.ERRINJ_DISCOVERY = errcnt - 1
+ if errcnt % 2 == 0 then
+ box.error(box.error.INJECTION, 'discovery')
+ end
+ else
+ M.errinj.ERRINJ_DISCOVERY = false
+ end
+ end
-- No way to select by {status, id}, because there are two
-- statuses to select. A router would need to maintain a
-- separate iterator for each status it wants to get. This may
====================
I couldn't come up with a sane test on whether router continues
discovery from exactly the same place where it got an error. I would
need to insert some extraordinary weird injections into router to
collect statistics of that kind. But the test at least shows, that an
error does not prevent full discovery eventually.
>> However, still there is space for improvement. Discovery could
>> avoid downloading anything after all is downloaded, if it could
>> somehow see, if bucket space is not changed. Unfortunately it is
>> not so easy, since bucket generation (version of _bucket space)
>> is not persisted. So after instance restart it is always equal to
>> bucket count.
>
> Is there an issue for that?
I was hoping you wouldn't ask :D
I don't really see how that optimization could be implemented in a sane
way in terms of simplicity. Looks like a storage would need to send
a bucket generation + some timestamp to protect form the case when
after restart bucket set is different, but generation is the same.
I was going to wait until somebody explicitly asks for it, if
'discovery_mode = "once"' won't be enough.
But ok, I was caught. Here is the issue:
https://github.com/tarantool/vshard/issues/238
>> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
>> index 6d88153..26ea85b 100644
>> --- a/vshard/router/init.lua
>> +++ b/vshard/router/init.lua
>> @@ -250,50 +250,127 @@ if util.version_is_at_least(1, 10, 0) then
>> discovery_f = function(router)
>> local module_version = M.module_version
>> assert(router.discovery_mode == 'on')
>> + local iterators = {}
>> + local opts = {is_async = true}
>> + local mode
>> while module_version == M.module_version do
>> - while not next(router.replicasets) do
>> - lfiber.sleep(consts.DISCOVERY_INTERVAL)
>> - end
>> - if module_version ~= M.module_version then
>> - return
>> - end
>> -- Just typical map reduce - send request to each
>> - -- replicaset in parallel, and collect responses.
>> - local pending = {}
>> - local opts = {is_async = true}
>> - local args = {}
>> - for rs_uuid, replicaset in pairs(router.replicasets) do
>> + -- replicaset in parallel, and collect responses. Many
>> + -- requests probably will be needed for each replicaset.
>> + --
>> + -- Step 1: create missing iterators, in case this is a
>> + -- first discovery iteration, or some replicasets were
>> + -- added after the router is started.
>> + for rs_uuid in pairs(router.replicasets) do
>> + local iter = iterators[rs_uuid]
>> + if not iter then
>> + iterators[rs_uuid] = {
>> + args = {{from = 1}},
>> + future = nil,
>> + }
>> + end
>> + end
>> + -- Step 2: map stage - send parallel requests for every
>> + -- iterator, prune orphan iterators whose replicasets were
>> + -- removed.
>> + for rs_uuid, iter in pairs(iterators) do
>> + local replicaset = router.replicasets[rs_uuid]
>> + if not replicaset then
>> + log.warn('Replicaset %s was removed during discovery', rs_uuid)
>> + iterators[rs_uuid] = nil
>> + goto continue
>> + end
>> local future, err =
>> - replicaset:callro('vshard.storage.buckets_discovery',
>> - args, opts)
>> + replicaset:callro('vshard.storage.buckets_discovery', iter.args,
>> + opts)
>> if not future then
>> - log.warn('Error during discovery %s: %s', rs_uuid, err)
>> - else
>> - pending[rs_uuid] = future
>> + log.warn('Error during discovery %s, retry will be done '..
>> + 'later: %s', rs_uuid, err)
>> + goto continue
>> + end
>> + iter.future = future
>> + -- Don't spam many requests at once. Give
>> + -- storages time to handle them and other
>> + -- requests.
>> + lfiber.sleep(consts.DISCOVERY_WORK_STEP)
>> + if module_version ~= M.module_version then
>> + return
>> end
>
>
> Is it possible to place such checks to some "common" places. At start/end of each iteration or between steps. This looks strange and a bit unobvous to do such checks after each timeout.
Purpose is to stop the fiber as soon as possible in case a reload
happened. So moving it to a common place is not really possible.
It would make it work longer with the old code after reload. But
if we keep the checks, I still need to do then after/before any
long yield.
>> + ::continue::
>> end
>> -
>> - local deadline = lfiber.clock() + consts.DISCOVERY_INTERVAL
>> - for rs_uuid, p in pairs(pending) do
>> + -- Step 3: reduce stage - collect responses, restart
>> + -- iterators which reached the end.
>> + for rs_uuid, iter in pairs(iterators) do
>> lfiber.yield()
>> - local timeout = deadline - lfiber.clock()
>> - local buckets, err = p:wait_result(timeout)
>> - while M.errinj.ERRINJ_LONG_DISCOVERY do
>> - M.errinj.ERRINJ_LONG_DISCOVERY = 'waiting'
>> - lfiber.sleep(0.01)
>> + local future = iter.future
>> + if not future then
>> + goto continue
>> end
>> - local replicaset = router.replicasets[rs_uuid]
>> - if not buckets then
>> - p:discard()
>> - log.warn('Error during discovery %s: %s', rs_uuid, err)
>> - elseif module_version ~= M.module_version then
>> + local result, err = future:wait_result(consts.DISCOVERY_TIMEOUT)
>> + if module_version ~= M.module_version then
>
> Does it have sence to call "discard" before "return"?
Yes. Discard removes the entry from netbox's internal table of requests
waiting for a result. I don't know how long the storage won't respond on
that, so better free the memory now. The entry is not removed automatically
in case it was a timeout error.
>> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
>> index 73c6740..0050b96 100644
>> --- a/vshard/storage/init.lua
>> +++ b/vshard/storage/init.lua
>> @@ -1110,10 +1110,47 @@ local function bucket_collect(bucket_id)
>> return data
>> end
>> +-- Discovery used by routers. It returns limited number of
>> +-- buckets to avoid stalls when _bucket is huge.
>> +local function buckets_discovery_extended(opts)
>> + local limit = consts.BUCKET_CHUNK_SIZE
>> + local buckets = table.new(limit, 0)
>> + local active = consts.BUCKET.ACTIVE
>> + local pinned = consts.BUCKET.PINNED
>> + local next_from
>> + -- No way to select by {status, id}, because there are two
>> + -- statuses to select. A router would need to maintain a
>> + -- separate iterator for each status it wants to get. This may
>> + -- be implemented in future. But _bucket space anyway 99% of
>> + -- time contains only active and pinned buckets. So there is
>> + -- no big benefit in optimizing that. Perhaps a compound index
>> + -- {status, id} could help too.
>> + for _, bucket in box.space._bucket:pairs({opts.from},
>> + {iterator = box.index.GE}) do
>> + local status = bucket.status
>> + if status == active or status == pinned then
>> + table.insert(buckets, bucket.id)
>> + end
>> + limit = limit - 1
>
> It's a bit strange after words about "99% time", I propose to move limit decrease under if condition.
But still there is this 1%, when buckets start moving. When bucket count
is millions, rebalancing is likely to move thousands of buckets, likely
from one range. For example, buckets from 1 to 10k can be moved. If in
that moment will arrive a discovery request, it will stuck for a long time
iterating over the sent and garbage buckets.
Rebalancing is a heavy thing, and such additional load would make the
cluster feel even worse.
99% here is rather about that it is not a problem to sometimes return an
empty bucket set. Not about allowing perf problems in the other 1%.
During working on your comments I found that during aggressive discovery I
added sleep in a wrong place. Fixed below:
====================
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index 26ea85b..28437e3 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -358,11 +358,14 @@ discovery_f = function(router)
mode = 'idle'
end
lfiber.sleep(consts.DISCOVERY_IDLE_INTERVAL)
- elseif mode ~= 'aggressive' then
- log.info('Start aggressive discovery, %s buckets are unknown. '..
- 'Discovery works with %s seconds interval',
- unknown_bucket_count, consts.DISCOVERY_WORK_INTERVAL)
- mode = 'aggressive'
+ else
+ if mode ~= 'aggressive' then
+ log.info('Start aggressive discovery, %s buckets are '..
+ 'unknown. Discovery works with %s seconds '..
+ 'interval', unknown_bucket_count,
+ consts.DISCOVERY_WORK_INTERVAL)
+ mode = 'aggressive'
+ end
lfiber.sleep(consts.DISCOVERY_WORK_INTERVAL)
break
end
====================
Since the changes are big, below is the whole new commit:
====================
router: make discovery smoother in a big cluster
Router does discovery once per 10 seconds. Discovery sends a
request to each replicaset to download all pinned and active
buckets from there. When there are millions of buckets, that
becomes a long operation taking seconds, during which the storage
is unresponsive.
The patch makes discovery work step by step, downloading not more
than 1000 buckets at a time. That gives the storage time to
process other requests.
Moreover, discovery now has some kind of 'state'. For each
replicaset it keeps an iterator which is moved by 1k buckets on
every successfully discovered bucket batch. It means, that if on a
replicaset with 1 000 000 buckets discovery fails after 999 999
buckets are already discovered, it won't start from 0. It will
retry from the old position.
However, still there is space for improvement. Discovery could
avoid downloading anything after all is downloaded, if it could
somehow see, if bucket space is not changed. Unfortunately it is
not so easy, since bucket generation (version of _bucket space)
is not persisted. So after instance restart it is always equal to
bucket count.
Part of #210
diff --git a/test/router/reload.result b/test/router/reload.result
index 3ba900a..8fe99ba 100644
--- a/test/router/reload.result
+++ b/test/router/reload.result
@@ -44,7 +44,10 @@ vshard.router.bootstrap()
while test_run:grep_log('router_1', 'All replicas are ok') == nil do fiber.sleep(0.1) end
---
...
-while test_run:grep_log('router_1', 'buckets: was 0, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
+while test_run:grep_log('router_1', 'buckets: was 0, became 1000') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
+---
+...
+while test_run:grep_log('router_1', 'buckets: was 1000, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
---
...
--
diff --git a/test/router/reload.test.lua b/test/router/reload.test.lua
index 5ed5690..abcbc09 100644
--- a/test/router/reload.test.lua
+++ b/test/router/reload.test.lua
@@ -15,7 +15,8 @@ fiber = require('fiber')
vshard.router.bootstrap()
while test_run:grep_log('router_1', 'All replicas are ok') == nil do fiber.sleep(0.1) end
-while test_run:grep_log('router_1', 'buckets: was 0, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
+while test_run:grep_log('router_1', 'buckets: was 0, became 1000') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
+while test_run:grep_log('router_1', 'buckets: was 1000, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
--
-- Gh-72: allow reload. Test simple reload, error during
diff --git a/test/router/router.result b/test/router/router.result
index df7be4a..b2efd6d 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -943,9 +943,9 @@ calculate_known_buckets()
---
- 3000
...
-test_run:grep_log('router_1', 'was 1, became 1500')
+test_run:grep_log('router_1', 'was 1, became 1000')
---
-- was 1, became 1500
+- was 1, became 1000
...
info = vshard.router.info()
---
diff --git a/test/router/router.test.lua b/test/router/router.test.lua
index 97dce49..154310b 100644
--- a/test/router/router.test.lua
+++ b/test/router/router.test.lua
@@ -316,7 +316,7 @@ vshard.storage.bucket_pin(first_active)
_ = test_run:switch('router_1')
wait_discovery()
calculate_known_buckets()
-test_run:grep_log('router_1', 'was 1, became 1500')
+test_run:grep_log('router_1', 'was 1, became 1000')
info = vshard.router.info()
info.bucket
info.alerts
diff --git a/test/router/router2.result b/test/router/router2.result
index 556f749..0ad5a21 100644
--- a/test/router/router2.result
+++ b/test/router/router2.result
@@ -123,6 +123,66 @@ f1:status(), f2, f3:status(), f4:status(), f5, f6:status()
| - suspended
| ...
+-- Errored discovery continued successfully after errors are gone.
+vshard.router.bootstrap()
+ | ---
+ | - true
+ | ...
+vshard.router.discovery_set('off')
+ | ---
+ | ...
+vshard.router._route_map_clear()
+ | ---
+ | ...
+
+-- Discovery requests 2 and 4 will fail on storages.
+util.map_evals(test_run, {{'storage_1_a'}, {'storage_2_a'}}, \
+ 'vshard.storage.internal.errinj.ERRINJ_DISCOVERY = 4')
+ | ---
+ | ...
+
+vshard.router.info().bucket.unknown
+ | ---
+ | - 3000
+ | ...
+vshard.router.discovery_set('on')
+ | ---
+ | ...
+function continue_discovery() \
+ local res = vshard.router.info().bucket.unknown == 0 \
+ if not res then \
+ vshard.router.discovery_wakeup() \
+ end \
+ return res \
+end
+ | ---
+ | ...
+test_run:wait_cond(continue_discovery)
+ | ---
+ | - true
+ | ...
+vshard.router.info().bucket.unknown
+ | ---
+ | - 0
+ | ...
+
+-- Discovery injections should be reset meaning they were returned
+-- needed number of times.
+_ = test_run:switch('storage_1_a')
+ | ---
+ | ...
+vshard.storage.internal.errinj.ERRINJ_DISCOVERY
+ | ---
+ | - 0
+ | ...
+_ = test_run:switch('storage_2_a')
+ | ---
+ | ...
+vshard.storage.internal.errinj.ERRINJ_DISCOVERY
+ | ---
+ | - 0
+ | ...
+
_ = test_run:switch("default")
| ---
| ...
diff --git a/test/router/router2.test.lua b/test/router/router2.test.lua
index 33f4d3e..ef05f8c 100644
--- a/test/router/router2.test.lua
+++ b/test/router/router2.test.lua
@@ -43,6 +43,34 @@ vshard.router.static.discovery_fiber:status()
f1:status(), f2, f3:status(), f4:status(), f5, f6:status()
+-- Errored discovery continued successfully after errors are gone.
+vshard.router.bootstrap()
+vshard.router.discovery_set('off')
+vshard.router._route_map_clear()
+
+-- Discovery requests 2 and 4 will fail on storages.
+util.map_evals(test_run, {{'storage_1_a'}, {'storage_2_a'}}, \
+ 'vshard.storage.internal.errinj.ERRINJ_DISCOVERY = 4')
+
+vshard.router.info().bucket.unknown
+vshard.router.discovery_set('on')
+function continue_discovery() \
+ local res = vshard.router.info().bucket.unknown == 0 \
+ if not res then \
+ vshard.router.discovery_wakeup() \
+ end \
+ return res \
+end
+test_run:wait_cond(continue_discovery)
+vshard.router.info().bucket.unknown
+
+-- Discovery injections should be reset meaning they were returned
+-- needed number of times.
+_ = test_run:switch('storage_1_a')
+vshard.storage.internal.errinj.ERRINJ_DISCOVERY
+_ = test_run:switch('storage_2_a')
+vshard.storage.internal.errinj.ERRINJ_DISCOVERY
+
_ = test_run:switch("default")
_ = test_run:cmd("stop server router_1")
_ = test_run:cmd("cleanup server router_1")
diff --git a/test/router/wrong_config.result b/test/router/wrong_config.result
index 56db9e8..92353c3 100644
--- a/test/router/wrong_config.result
+++ b/test/router/wrong_config.result
@@ -45,7 +45,10 @@ cfg.bucket_count = 1000
r = vshard.router.new('gh-179', cfg)
---
...
-while type(r:info().bucket.unknown) == 'number' and r:info().bucket.unknown > 0 do r:discovery_wakeup() fiber.sleep(0.1) end
+while r:info().bucket.available_rw ~= 3000 do \
+ r:discovery_wakeup() \
+ fiber.sleep(0.1) \
+end
---
...
i = r:info()
diff --git a/test/router/wrong_config.test.lua b/test/router/wrong_config.test.lua
index 62ef30d..174b373 100644
--- a/test/router/wrong_config.test.lua
+++ b/test/router/wrong_config.test.lua
@@ -18,7 +18,10 @@ vshard.router.bootstrap()
--
cfg.bucket_count = 1000
r = vshard.router.new('gh-179', cfg)
-while type(r:info().bucket.unknown) == 'number' and r:info().bucket.unknown > 0 do r:discovery_wakeup() fiber.sleep(0.1) end
+while r:info().bucket.available_rw ~= 3000 do \
+ r:discovery_wakeup() \
+ fiber.sleep(0.1) \
+end
i = r:info()
i.bucket
i.alerts
diff --git a/vshard/consts.lua b/vshard/consts.lua
index 5391c0f..a6a8c1b 100644
--- a/vshard/consts.lua
+++ b/vshard/consts.lua
@@ -40,5 +40,8 @@ return {
DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL = 0.5;
RECOVERY_INTERVAL = 5;
COLLECT_LUA_GARBAGE_INTERVAL = 100;
- DISCOVERY_INTERVAL = 10;
+ DISCOVERY_IDLE_INTERVAL = 10,
+ DISCOVERY_WORK_INTERVAL = 1,
+ DISCOVERY_WORK_STEP = 0.01,
+ DISCOVERY_TIMEOUT = 10,
}
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index 6d88153..28437e3 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -250,50 +250,130 @@ if util.version_is_at_least(1, 10, 0) then
discovery_f = function(router)
local module_version = M.module_version
assert(router.discovery_mode == 'on')
+ local iterators = {}
+ local opts = {is_async = true}
+ local mode
while module_version == M.module_version do
- while not next(router.replicasets) do
- lfiber.sleep(consts.DISCOVERY_INTERVAL)
- end
- if module_version ~= M.module_version then
- return
- end
-- Just typical map reduce - send request to each
- -- replicaset in parallel, and collect responses.
- local pending = {}
- local opts = {is_async = true}
- local args = {}
- for rs_uuid, replicaset in pairs(router.replicasets) do
+ -- replicaset in parallel, and collect responses. Many
+ -- requests probably will be needed for each replicaset.
+ --
+ -- Step 1: create missing iterators, in case this is a
+ -- first discovery iteration, or some replicasets were
+ -- added after the router is started.
+ for rs_uuid in pairs(router.replicasets) do
+ local iter = iterators[rs_uuid]
+ if not iter then
+ iterators[rs_uuid] = {
+ args = {{from = 1}},
+ future = nil,
+ }
+ end
+ end
+ -- Step 2: map stage - send parallel requests for every
+ -- iterator, prune orphan iterators whose replicasets were
+ -- removed.
+ for rs_uuid, iter in pairs(iterators) do
+ local replicaset = router.replicasets[rs_uuid]
+ if not replicaset then
+ log.warn('Replicaset %s was removed during discovery', rs_uuid)
+ iterators[rs_uuid] = nil
+ goto continue
+ end
local future, err =
- replicaset:callro('vshard.storage.buckets_discovery',
- args, opts)
+ replicaset:callro('vshard.storage.buckets_discovery', iter.args,
+ opts)
if not future then
- log.warn('Error during discovery %s: %s', rs_uuid, err)
- else
- pending[rs_uuid] = future
+ log.warn('Error during discovery %s, retry will be done '..
+ 'later: %s', rs_uuid, err)
+ goto continue
end
+ iter.future = future
+ -- Don't spam many requests at once. Give
+ -- storages time to handle them and other
+ -- requests.
+ lfiber.sleep(consts.DISCOVERY_WORK_STEP)
+ if module_version ~= M.module_version then
+ return
+ end
+ ::continue::
end
-
- local deadline = lfiber.clock() + consts.DISCOVERY_INTERVAL
- for rs_uuid, p in pairs(pending) do
+ -- Step 3: reduce stage - collect responses, restart
+ -- iterators which reached the end.
+ for rs_uuid, iter in pairs(iterators) do
lfiber.yield()
- local timeout = deadline - lfiber.clock()
- local buckets, err = p:wait_result(timeout)
- while M.errinj.ERRINJ_LONG_DISCOVERY do
- M.errinj.ERRINJ_LONG_DISCOVERY = 'waiting'
- lfiber.sleep(0.01)
+ local future = iter.future
+ if not future then
+ goto continue
end
- local replicaset = router.replicasets[rs_uuid]
- if not buckets then
- p:discard()
- log.warn('Error during discovery %s: %s', rs_uuid, err)
- elseif module_version ~= M.module_version then
+ local result, err = future:wait_result(consts.DISCOVERY_TIMEOUT)
+ if module_version ~= M.module_version then
return
- elseif replicaset then
- discovery_handle_buckets(router, replicaset, buckets[1])
end
+ if not result then
+ future:discard()
+ log.warn('Error during discovery %s, retry will be done '..
+ 'later: %s', rs_uuid, err)
+ goto continue
+ end
+ local replicaset = router.replicasets[rs_uuid]
+ if not replicaset then
+ iterators[rs_uuid] = nil
+ log.warn('Replicaset %s was removed during discovery', rs_uuid)
+ goto continue
+ end
+ result = result[1]
+ -- Buckets are returned as plain array by storages
+ -- using old vshard version. But if .buckets is set,
+ -- this is a new storage.
+ discovery_handle_buckets(router, replicaset,
+ result.buckets or result)
+ local discovery_args = iter.args[1]
+ discovery_args.from = result.next_from
+ if not result.next_from then
+ -- Nil next_from means no more buckets to get.
+ -- Restart the iterator.
+ iterators[rs_uuid] = nil
+ end
+ ::continue::
end
-
- lfiber.sleep(deadline - lfiber.clock())
+ local unknown_bucket_count
+ repeat
+ unknown_bucket_count =
+ router.total_bucket_count - router.known_bucket_count
+ if unknown_bucket_count == 0 then
+ if mode ~= 'idle' then
+ log.info('Discovery enters idle mode, all buckets are '..
+ 'known. Discovery works with %s seconds '..
+ 'interval now', consts.DISCOVERY_IDLE_INTERVAL)
+ mode = 'idle'
+ end
+ lfiber.sleep(consts.DISCOVERY_IDLE_INTERVAL)
+ elseif not next(router.replicasets) then
+ if mode ~= 'idle' then
+ log.info('Discovery enters idle mode because '..
+ 'configuration does not have replicasets. '..
+ 'Retries will happen with %s seconds interval',
+ consts.DISCOVERY_IDLE_INTERVAL)
+ mode = 'idle'
+ end
+ lfiber.sleep(consts.DISCOVERY_IDLE_INTERVAL)
+ else
+ if mode ~= 'aggressive' then
+ log.info('Start aggressive discovery, %s buckets are '..
+ 'unknown. Discovery works with %s seconds '..
+ 'interval', unknown_bucket_count,
+ consts.DISCOVERY_WORK_INTERVAL)
+ mode = 'aggressive'
+ end
+ lfiber.sleep(consts.DISCOVERY_WORK_INTERVAL)
+ break
+ end
+ while M.errinj.ERRINJ_LONG_DISCOVERY do
+ M.errinj.ERRINJ_LONG_DISCOVERY = 'waiting'
+ lfiber.sleep(0.01)
+ end
+ until next(router.replicasets)
end
end
@@ -355,9 +435,9 @@ local function discovery_set(router, new_mode)
router.discovery_mode = new_mode
if router.discovery_fiber ~= nil then
pcall(router.discovery_fiber.cancel, router.discovery_fiber)
+ router.discovery_fiber = nil
end
if new_mode == 'off' then
- router.discovery_fiber = nil
return
end
router.discovery_fiber = util.reloadable_fiber_create(
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
index 73c6740..c6a78fe 100644
--- a/vshard/storage/init.lua
+++ b/vshard/storage/init.lua
@@ -75,6 +75,7 @@ if not M then
ERRINJ_RECEIVE_PARTIALLY = false,
ERRINJ_NO_RECOVERY = false,
ERRINJ_UPGRADE = false,
+ ERRINJ_DISCOVERY = false,
},
-- This counter is used to restart background fibers with
-- new reloaded code.
@@ -1110,10 +1111,58 @@ local function bucket_collect(bucket_id)
return data
end
+-- Discovery used by routers. It returns limited number of
+-- buckets to avoid stalls when _bucket is huge.
+local function buckets_discovery_extended(opts)
+ local limit = consts.BUCKET_CHUNK_SIZE
+ local buckets = table.new(limit, 0)
+ local active = consts.BUCKET.ACTIVE
+ local pinned = consts.BUCKET.PINNED
+ local next_from
+ local errcnt = M.errinj.ERRINJ_DISCOVERY
+ if errcnt then
+ if errcnt > 0 then
+ M.errinj.ERRINJ_DISCOVERY = errcnt - 1
+ if errcnt % 2 == 0 then
+ box.error(box.error.INJECTION, 'discovery')
+ end
+ else
+ M.errinj.ERRINJ_DISCOVERY = false
+ end
+ end
+ -- No way to select by {status, id}, because there are two
+ -- statuses to select. A router would need to maintain a
+ -- separate iterator for each status it wants to get. This may
+ -- be implemented in future. But _bucket space anyway 99% of
+ -- time contains only active and pinned buckets. So there is
+ -- no big benefit in optimizing that. Perhaps a compound index
+ -- {status, id} could help too.
+ for _, bucket in box.space._bucket:pairs({opts.from},
+ {iterator = box.index.GE}) do
+ local status = bucket.status
+ if status == active or status == pinned then
+ table.insert(buckets, bucket.id)
+ end
+ limit = limit - 1
+ if limit == 0 then
+ next_from = bucket.id + 1
+ break
+ end
+ end
+ -- Buckets list can even be empty, if all buckets in the
+ -- scanned chunk are not active/pinned. But next_from still
+ -- should be returned. So as the router could request more.
+ return {buckets = buckets, next_from = next_from}
+end
+
--
-- Collect array of active bucket identifiers for discovery.
--
-local function buckets_discovery()
+local function buckets_discovery(opts)
+ if opts then
+ -- Private method. Is not documented intentionally.
+ return buckets_discovery_extended(opts)
+ end
local ret = {}
local status = box.space._bucket.index.status
for _, bucket in status:pairs({consts.BUCKET.ACTIVE}) do
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster
2020-05-02 20:12 ` Vladislav Shpilevoy
@ 2020-05-04 14:26 ` Oleg Babin
2020-05-04 21:09 ` Vladislav Shpilevoy
0 siblings, 1 reply; 29+ messages in thread
From: Oleg Babin @ 2020-05-04 14:26 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Thanks for answers and changes! LGTM.
I left my answers and one nit to new patch diff.
On 02/05/2020 23:12, Vladislav Shpilevoy wrote:
> Thanks for the review!
>
> On 01/05/2020 19:01, Oleg Babin wrote:
>> Thanks for your patch! See my comments below.
>>
>> On 01/05/2020 03:16, Vladislav Shpilevoy wrote:
>>> Router does discovery once per 10 seconds. Discovery sends a
>>> request to each replicaset to download all pinned and active
>>> buckets from there. When there are millions of buckets, that
>>> becomes a long operation taking seconds, during which the storage
>>> is unresponsive.
>>>
>>> The patch makes discovery work step by step, downloading not more
>>> than 1000 buckets at a time. That gives the storage time to
>>> process other requests.
>>>
>>> Moreover, discovery now has some kind of 'state'. For each
>>> replicaset it keeps an iterator which is moved by 1k buckets on
>>> every successfully discovered bucket batch. It means, that if on a
>>> replicaset with 1 000 000 buckets discovery fails after 999 999
>>> buckets are already discovered, it won't start from 0. It will
>>> retry from the old position.
>>
>> Could you provide a test for such case?
>
> No problem.
> ... > I couldn't come up with a sane test on whether router continues
> discovery from exactly the same place where it got an error. I would
> need to insert some extraordinary weird injections into router to
> collect statistics of that kind. But the test at least shows, that an
> error does not prevent full discovery eventually.
>
Thanks, I believe it's enough.
>>> However, still there is space for improvement. Discovery could
>>> avoid downloading anything after all is downloaded, if it could
>>> somehow see, if bucket space is not changed. Unfortunately it is
>>> not so easy, since bucket generation (version of _bucket space)
>>> is not persisted. So after instance restart it is always equal to
>>> bucket count.
>>
>> Is there an issue for that?
>
> I was hoping you wouldn't ask :D
>
> I don't really see how that optimization could be implemented in a sane
> way in terms of simplicity. Looks like a storage would need to send
> a bucket generation + some timestamp to protect form the case when
> after restart bucket set is different, but generation is the same.
>
> I was going to wait until somebody explicitly asks for it, if
> 'discovery_mode = "once"' won't be enough.
>
> But ok, I was caught. Here is the issue:
> https://github.com/tarantool/vshard/issues/238
>
Hah, I don't know but when I read this part of commit message I thought
about trigger on the "_bucket" space that could update some "generation"
value may be in "_schema" space. I don't know could it be appropriate
way to implement this feature but I asked to make sure that this propose
won't be lost.
>>> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
>>> index 6d88153..26ea85b 100644
>>> --- a/vshard/router/init.lua
>>> +++ b/vshard/router/init.lua
>>> @@ -250,50 +250,127 @@ if util.version_is_at_least(1, 10, 0) then
>>> discovery_f = function(router)
>>> local module_version = M.module_version
>>> assert(router.discovery_mode == 'on')
>>> + local iterators = {}
>>> + local opts = {is_async = true}
>>> + local mode
>>> while module_version == M.module_version do
>>> - while not next(router.replicasets) do
>>> - lfiber.sleep(consts.DISCOVERY_INTERVAL)
>>> - end
>>> - if module_version ~= M.module_version then
>>> - return
>>> - end
>>> -- Just typical map reduce - send request to each
>>> - -- replicaset in parallel, and collect responses.
>>> - local pending = {}
>>> - local opts = {is_async = true}
>>> - local args = {}
>>> - for rs_uuid, replicaset in pairs(router.replicasets) do
>>> + -- replicaset in parallel, and collect responses. Many
>>> + -- requests probably will be needed for each replicaset.
>>> + --
>>> + -- Step 1: create missing iterators, in case this is a
>>> + -- first discovery iteration, or some replicasets were
>>> + -- added after the router is started.
>>> + for rs_uuid in pairs(router.replicasets) do
>>> + local iter = iterators[rs_uuid]
>>> + if not iter then
>>> + iterators[rs_uuid] = {
>>> + args = {{from = 1}},
>>> + future = nil,
>>> + }
>>> + end
>>> + end
>>> + -- Step 2: map stage - send parallel requests for every
>>> + -- iterator, prune orphan iterators whose replicasets were
>>> + -- removed.
>>> + for rs_uuid, iter in pairs(iterators) do
>>> + local replicaset = router.replicasets[rs_uuid]
>>> + if not replicaset then
>>> + log.warn('Replicaset %s was removed during discovery', rs_uuid)
>>> + iterators[rs_uuid] = nil
>>> + goto continue
>>> + end
>>> local future, err =
>>> - replicaset:callro('vshard.storage.buckets_discovery',
>>> - args, opts)
>>> + replicaset:callro('vshard.storage.buckets_discovery', iter.args,
>>> + opts)
>>> if not future then
>>> - log.warn('Error during discovery %s: %s', rs_uuid, err)
>>> - else
>>> - pending[rs_uuid] = future
>>> + log.warn('Error during discovery %s, retry will be done '..
>>> + 'later: %s', rs_uuid, err)
>>> + goto continue
>>> + end
>>> + iter.future = future
>>> + -- Don't spam many requests at once. Give
>>> + -- storages time to handle them and other
>>> + -- requests.
>>> + lfiber.sleep(consts.DISCOVERY_WORK_STEP)
>>> + if module_version ~= M.module_version then
>>> + return
>>> end
>>
>>
>> Is it possible to place such checks to some "common" places. At start/end of each iteration or between steps. This looks strange and a bit unobvous to do such checks after each timeout.
>
> Purpose is to stop the fiber as soon as possible in case a reload
> happened. So moving it to a common place is not really possible.
> It would make it work longer with the old code after reload. But
> if we keep the checks, I still need to do then after/before any
> long yield.
>
My main concern is that such things could be easily missed in future.
But I agree that we should left this function early as possible.
>>> + ::continue::
>>> end
>>> -
>>> - local deadline = lfiber.clock() + consts.DISCOVERY_INTERVAL
>>> - for rs_uuid, p in pairs(pending) do
>>> + -- Step 3: reduce stage - collect responses, restart
>>> + -- iterators which reached the end.
>>> + for rs_uuid, iter in pairs(iterators) do
>>> lfiber.yield()
>>> - local timeout = deadline - lfiber.clock()
>>> - local buckets, err = p:wait_result(timeout)
>>> - while M.errinj.ERRINJ_LONG_DISCOVERY do
>>> - M.errinj.ERRINJ_LONG_DISCOVERY = 'waiting'
>>> - lfiber.sleep(0.01)
>>> + local future = iter.future
>>> + if not future then
>>> + goto continue
>>> end
>>> - local replicaset = router.replicasets[rs_uuid]
>>> - if not buckets then
>>> - p:discard()
>>> - log.warn('Error during discovery %s: %s', rs_uuid, err)
>>> - elseif module_version ~= M.module_version then
>>> + local result, err = future:wait_result(consts.DISCOVERY_TIMEOUT)
>>> + if module_version ~= M.module_version then
>>
>> Does it have sence to call "discard" before "return"?
>
> Yes. Discard removes the entry from netbox's internal table of requests
> waiting for a result. I don't know how long the storage won't respond on
> that, so better free the memory now. The entry is not removed automatically
> in case it was a timeout error.
>
>>> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
>>> index 73c6740..0050b96 100644
>>> --- a/vshard/storage/init.lua
>>> +++ b/vshard/storage/init.lua
>>> @@ -1110,10 +1110,47 @@ local function bucket_collect(bucket_id)
>>> return data
>>> end
>>> +-- Discovery used by routers. It returns limited number of
>>> +-- buckets to avoid stalls when _bucket is huge.
>>> +local function buckets_discovery_extended(opts)
>>> + local limit = consts.BUCKET_CHUNK_SIZE
>>> + local buckets = table.new(limit, 0)
>>> + local active = consts.BUCKET.ACTIVE
>>> + local pinned = consts.BUCKET.PINNED
>>> + local next_from
>>> + -- No way to select by {status, id}, because there are two
>>> + -- statuses to select. A router would need to maintain a
>>> + -- separate iterator for each status it wants to get. This may
>>> + -- be implemented in future. But _bucket space anyway 99% of
>>> + -- time contains only active and pinned buckets. So there is
>>> + -- no big benefit in optimizing that. Perhaps a compound index
>>> + -- {status, id} could help too.
>>> + for _, bucket in box.space._bucket:pairs({opts.from},
>>> + {iterator = box.index.GE}) do
>>> + local status = bucket.status
>>> + if status == active or status == pinned then
>>> + table.insert(buckets, bucket.id)
>>> + end
>>> + limit = limit - 1
>>
>> It's a bit strange after words about "99% time", I propose to move limit decrease under if condition.
>
> But still there is this 1%, when buckets start moving. When bucket count
> is millions, rebalancing is likely to move thousands of buckets, likely
> from one range. For example, buckets from 1 to 10k can be moved. If in
> that moment will arrive a discovery request, it will stuck for a long time
> iterating over the sent and garbage buckets.
>
> Rebalancing is a heavy thing, and such additional load would make the
> cluster feel even worse.
>
> 99% here is rather about that it is not a problem to sometimes return an
> empty bucket set. Not about allowing perf problems in the other 1%.
>
Agree
>
> During working on your comments I found that during aggressive discovery I
> added sleep in a wrong place. Fixed below:
>
> ====================
> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> index 26ea85b..28437e3 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -358,11 +358,14 @@ discovery_f = function(router)
> mode = 'idle'
> end
> lfiber.sleep(consts.DISCOVERY_IDLE_INTERVAL)
> - elseif mode ~= 'aggressive' then
> - log.info('Start aggressive discovery, %s buckets are unknown. '..
> - 'Discovery works with %s seconds interval',
> - unknown_bucket_count, consts.DISCOVERY_WORK_INTERVAL)
> - mode = 'aggressive'
> + else
> + if mode ~= 'aggressive' then
> + log.info('Start aggressive discovery, %s buckets are '..
> + 'unknown. Discovery works with %s seconds '..
> + 'interval', unknown_bucket_count,
> + consts.DISCOVERY_WORK_INTERVAL)
> + mode = 'aggressive'
> + end
> lfiber.sleep(consts.DISCOVERY_WORK_INTERVAL)
> break
> end
>
> ====================
>
> Since the changes are big, below is the whole new commit:
>
> ====================
>
> router: make discovery smoother in a big cluster
>
> Router does discovery once per 10 seconds. Discovery sends a
> request to each replicaset to download all pinned and active
> buckets from there. When there are millions of buckets, that
> becomes a long operation taking seconds, during which the storage
> is unresponsive.
>
> The patch makes discovery work step by step, downloading not more
> than 1000 buckets at a time. That gives the storage time to
> process other requests.
>
> Moreover, discovery now has some kind of 'state'. For each
> replicaset it keeps an iterator which is moved by 1k buckets on
> every successfully discovered bucket batch. It means, that if on a
> replicaset with 1 000 000 buckets discovery fails after 999 999
> buckets are already discovered, it won't start from 0. It will
> retry from the old position.
>
> However, still there is space for improvement. Discovery could
> avoid downloading anything after all is downloaded, if it could
> somehow see, if bucket space is not changed. Unfortunately it is
> not so easy, since bucket generation (version of _bucket space)
> is not persisted. So after instance restart it is always equal to
> bucket count.
>
> Part of #210
>
> diff --git a/test/router/reload.result b/test/router/reload.result
> index 3ba900a..8fe99ba 100644
> --- a/test/router/reload.result
> +++ b/test/router/reload.result
> @@ -44,7 +44,10 @@ vshard.router.bootstrap()
> while test_run:grep_log('router_1', 'All replicas are ok') == nil do fiber.sleep(0.1) end
> ---
> ...
> -while test_run:grep_log('router_1', 'buckets: was 0, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
> +while test_run:grep_log('router_1', 'buckets: was 0, became 1000') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
> +---
> +...
> +while test_run:grep_log('router_1', 'buckets: was 1000, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
> ---
> ...
> --
> diff --git a/test/router/reload.test.lua b/test/router/reload.test.lua
> index 5ed5690..abcbc09 100644
> --- a/test/router/reload.test.lua
> +++ b/test/router/reload.test.lua
> @@ -15,7 +15,8 @@ fiber = require('fiber')
> vshard.router.bootstrap()
>
> while test_run:grep_log('router_1', 'All replicas are ok') == nil do fiber.sleep(0.1) end
> -while test_run:grep_log('router_1', 'buckets: was 0, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
> +while test_run:grep_log('router_1', 'buckets: was 0, became 1000') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
> +while test_run:grep_log('router_1', 'buckets: was 1000, became 1500') == nil do fiber.sleep(0.1) vshard.router.discovery_wakeup() end
>
> --
> -- Gh-72: allow reload. Test simple reload, error during
> diff --git a/test/router/router.result b/test/router/router.result
> index df7be4a..b2efd6d 100644
> --- a/test/router/router.result
> +++ b/test/router/router.result
> @@ -943,9 +943,9 @@ calculate_known_buckets()
> ---
> - 3000
> ...
> -test_run:grep_log('router_1', 'was 1, became 1500')
> +test_run:grep_log('router_1', 'was 1, became 1000')
> ---
> -- was 1, became 1500
> +- was 1, became 1000
> ...
> info = vshard.router.info()
> ---
> diff --git a/test/router/router.test.lua b/test/router/router.test.lua
> index 97dce49..154310b 100644
> --- a/test/router/router.test.lua
> +++ b/test/router/router.test.lua
> @@ -316,7 +316,7 @@ vshard.storage.bucket_pin(first_active)
> _ = test_run:switch('router_1')
> wait_discovery()
> calculate_known_buckets()
> -test_run:grep_log('router_1', 'was 1, became 1500')
> +test_run:grep_log('router_1', 'was 1, became 1000')
> info = vshard.router.info()
> info.bucket
> info.alerts
> diff --git a/test/router/router2.result b/test/router/router2.result
> index 556f749..0ad5a21 100644
> --- a/test/router/router2.result
> +++ b/test/router/router2.result
> @@ -123,6 +123,66 @@ f1:status(), f2, f3:status(), f4:status(), f5, f6:status()
> | - suspended
> | ...
>
> +-- Errored discovery continued successfully after errors are gone.
> +vshard.router.bootstrap()
> + | ---
> + | - true
> + | ...
> +vshard.router.discovery_set('off')
> + | ---
> + | ...
> +vshard.router._route_map_clear()
> + | ---
> + | ...
> +
> +-- Discovery requests 2 and 4 will fail on storages.
> +util.map_evals(test_run, {{'storage_1_a'}, {'storage_2_a'}}, \
> + 'vshard.storage.internal.errinj.ERRINJ_DISCOVERY = 4')
> + | ---
> + | ...
> +
> +vshard.router.info().bucket.unknown
> + | ---
> + | - 3000
> + | ...
> +vshard.router.discovery_set('on')
> + | ---
> + | ...
> +function continue_discovery() \
> + local res = vshard.router.info().bucket.unknown == 0 \
> + if not res then \
> + vshard.router.discovery_wakeup() \
> + end \
> + return res \
> +end
> + | ---
> + | ...
> +test_run:wait_cond(continue_discovery)
> + | ---
> + | - true
> + | ...
> +vshard.router.info().bucket.unknown
> + | ---
> + | - 0
> + | ...
> +
> +-- Discovery injections should be reset meaning they were returned
> +-- needed number of times.
> +_ = test_run:switch('storage_1_a')
> + | ---
> + | ...
> +vshard.storage.internal.errinj.ERRINJ_DISCOVERY
> + | ---
> + | - 0
> + | ...
> +_ = test_run:switch('storage_2_a')
> + | ---
> + | ...
> +vshard.storage.internal.errinj.ERRINJ_DISCOVERY
> + | ---
> + | - 0
> + | ...
> +
> _ = test_run:switch("default")
> | ---
> | ...
> diff --git a/test/router/router2.test.lua b/test/router/router2.test.lua
> index 33f4d3e..ef05f8c 100644
> --- a/test/router/router2.test.lua
> +++ b/test/router/router2.test.lua
> @@ -43,6 +43,34 @@ vshard.router.static.discovery_fiber:status()
>
> f1:status(), f2, f3:status(), f4:status(), f5, f6:status()
>
> +-- Errored discovery continued successfully after errors are gone.
> +vshard.router.bootstrap()
> +vshard.router.discovery_set('off')
> +vshard.router._route_map_clear()
> +
> +-- Discovery requests 2 and 4 will fail on storages.
> +util.map_evals(test_run, {{'storage_1_a'}, {'storage_2_a'}}, \
> + 'vshard.storage.internal.errinj.ERRINJ_DISCOVERY = 4')
> +
> +vshard.router.info().bucket.unknown
> +vshard.router.discovery_set('on')
> +function continue_discovery() \
> + local res = vshard.router.info().bucket.unknown == 0 \
> + if not res then \
> + vshard.router.discovery_wakeup() \
> + end \
> + return res \
> +end
> +test_run:wait_cond(continue_discovery)
> +vshard.router.info().bucket.unknown
> +
> +-- Discovery injections should be reset meaning they were returned
> +-- needed number of times.
> +_ = test_run:switch('storage_1_a')
> +vshard.storage.internal.errinj.ERRINJ_DISCOVERY
> +_ = test_run:switch('storage_2_a')
> +vshard.storage.internal.errinj.ERRINJ_DISCOVERY
> +
> _ = test_run:switch("default")
> _ = test_run:cmd("stop server router_1")
> _ = test_run:cmd("cleanup server router_1")
> diff --git a/test/router/wrong_config.result b/test/router/wrong_config.result
> index 56db9e8..92353c3 100644
> --- a/test/router/wrong_config.result
> +++ b/test/router/wrong_config.result
> @@ -45,7 +45,10 @@ cfg.bucket_count = 1000
> r = vshard.router.new('gh-179', cfg)
> ---
> ...
> -while type(r:info().bucket.unknown) == 'number' and r:info().bucket.unknown > 0 do r:discovery_wakeup() fiber.sleep(0.1) end
> +while r:info().bucket.available_rw ~= 3000 do \
> + r:discovery_wakeup() \
> + fiber.sleep(0.1) \
> +end
> ---
> ...
> i = r:info()
> diff --git a/test/router/wrong_config.test.lua b/test/router/wrong_config.test.lua
> index 62ef30d..174b373 100644
> --- a/test/router/wrong_config.test.lua
> +++ b/test/router/wrong_config.test.lua
> @@ -18,7 +18,10 @@ vshard.router.bootstrap()
> --
> cfg.bucket_count = 1000
> r = vshard.router.new('gh-179', cfg)
> -while type(r:info().bucket.unknown) == 'number' and r:info().bucket.unknown > 0 do r:discovery_wakeup() fiber.sleep(0.1) end
> +while r:info().bucket.available_rw ~= 3000 do \
> + r:discovery_wakeup() \
> + fiber.sleep(0.1) \
> +end
> i = r:info()
> i.bucket
> i.alerts
> diff --git a/vshard/consts.lua b/vshard/consts.lua
> index 5391c0f..a6a8c1b 100644
> --- a/vshard/consts.lua
> +++ b/vshard/consts.lua
> @@ -40,5 +40,8 @@ return {
> DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL = 0.5;
> RECOVERY_INTERVAL = 5;
> COLLECT_LUA_GARBAGE_INTERVAL = 100;
> - DISCOVERY_INTERVAL = 10;
> + DISCOVERY_IDLE_INTERVAL = 10,
> + DISCOVERY_WORK_INTERVAL = 1,
> + DISCOVERY_WORK_STEP = 0.01,
> + DISCOVERY_TIMEOUT = 10,
> }
Nit: here commas and semicolons are mixed. I think we should support it
in consistent state - only commas or only semicolons.
> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> index 6d88153..28437e3 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -250,50 +250,130 @@ if util.version_is_at_least(1, 10, 0) then
> discovery_f = function(router)
> local module_version = M.module_version
> assert(router.discovery_mode == 'on')
> + local iterators = {}
> + local opts = {is_async = true}
> + local mode
> while module_version == M.module_version do
> - while not next(router.replicasets) do
> - lfiber.sleep(consts.DISCOVERY_INTERVAL)
> - end
> - if module_version ~= M.module_version then
> - return
> - end
> -- Just typical map reduce - send request to each
> - -- replicaset in parallel, and collect responses.
> - local pending = {}
> - local opts = {is_async = true}
> - local args = {}
> - for rs_uuid, replicaset in pairs(router.replicasets) do
> + -- replicaset in parallel, and collect responses. Many
> + -- requests probably will be needed for each replicaset.
> + --
> + -- Step 1: create missing iterators, in case this is a
> + -- first discovery iteration, or some replicasets were
> + -- added after the router is started.
> + for rs_uuid in pairs(router.replicasets) do
> + local iter = iterators[rs_uuid]
> + if not iter then
> + iterators[rs_uuid] = {
> + args = {{from = 1}},
> + future = nil,
> + }
> + end
> + end
> + -- Step 2: map stage - send parallel requests for every
> + -- iterator, prune orphan iterators whose replicasets were
> + -- removed.
> + for rs_uuid, iter in pairs(iterators) do
> + local replicaset = router.replicasets[rs_uuid]
> + if not replicaset then
> + log.warn('Replicaset %s was removed during discovery', rs_uuid)
> + iterators[rs_uuid] = nil
> + goto continue
> + end
> local future, err =
> - replicaset:callro('vshard.storage.buckets_discovery',
> - args, opts)
> + replicaset:callro('vshard.storage.buckets_discovery', iter.args,
> + opts)
> if not future then
> - log.warn('Error during discovery %s: %s', rs_uuid, err)
> - else
> - pending[rs_uuid] = future
> + log.warn('Error during discovery %s, retry will be done '..
> + 'later: %s', rs_uuid, err)
> + goto continue
> end
> + iter.future = future
> + -- Don't spam many requests at once. Give
> + -- storages time to handle them and other
> + -- requests.
> + lfiber.sleep(consts.DISCOVERY_WORK_STEP)
> + if module_version ~= M.module_version then
> + return
> + end
> + ::continue::
> end
> -
> - local deadline = lfiber.clock() + consts.DISCOVERY_INTERVAL
> - for rs_uuid, p in pairs(pending) do
> + -- Step 3: reduce stage - collect responses, restart
> + -- iterators which reached the end.
> + for rs_uuid, iter in pairs(iterators) do
> lfiber.yield()
> - local timeout = deadline - lfiber.clock()
> - local buckets, err = p:wait_result(timeout)
> - while M.errinj.ERRINJ_LONG_DISCOVERY do
> - M.errinj.ERRINJ_LONG_DISCOVERY = 'waiting'
> - lfiber.sleep(0.01)
> + local future = iter.future
> + if not future then
> + goto continue
> end
> - local replicaset = router.replicasets[rs_uuid]
> - if not buckets then
> - p:discard()
> - log.warn('Error during discovery %s: %s', rs_uuid, err)
> - elseif module_version ~= M.module_version then
> + local result, err = future:wait_result(consts.DISCOVERY_TIMEOUT)
> + if module_version ~= M.module_version then
> return
> - elseif replicaset then
> - discovery_handle_buckets(router, replicaset, buckets[1])
> end
> + if not result then
> + future:discard()
> + log.warn('Error during discovery %s, retry will be done '..
> + 'later: %s', rs_uuid, err)
> + goto continue
> + end
> + local replicaset = router.replicasets[rs_uuid]
> + if not replicaset then
> + iterators[rs_uuid] = nil
> + log.warn('Replicaset %s was removed during discovery', rs_uuid)
> + goto continue
> + end
> + result = result[1]
> + -- Buckets are returned as plain array by storages
> + -- using old vshard version. But if .buckets is set,
> + -- this is a new storage.
> + discovery_handle_buckets(router, replicaset,
> + result.buckets or result)
> + local discovery_args = iter.args[1]
> + discovery_args.from = result.next_from
> + if not result.next_from then
> + -- Nil next_from means no more buckets to get.
> + -- Restart the iterator.
> + iterators[rs_uuid] = nil
> + end
> + ::continue::
> end
> -
> - lfiber.sleep(deadline - lfiber.clock())
> + local unknown_bucket_count
> + repeat
> + unknown_bucket_count =
> + router.total_bucket_count - router.known_bucket_count
> + if unknown_bucket_count == 0 then
> + if mode ~= 'idle' then
> + log.info('Discovery enters idle mode, all buckets are '..
> + 'known. Discovery works with %s seconds '..
> + 'interval now', consts.DISCOVERY_IDLE_INTERVAL)
> + mode = 'idle'
> + end
> + lfiber.sleep(consts.DISCOVERY_IDLE_INTERVAL)
> + elseif not next(router.replicasets) then
> + if mode ~= 'idle' then
> + log.info('Discovery enters idle mode because '..
> + 'configuration does not have replicasets. '..
> + 'Retries will happen with %s seconds interval',
> + consts.DISCOVERY_IDLE_INTERVAL)
> + mode = 'idle'
> + end
> + lfiber.sleep(consts.DISCOVERY_IDLE_INTERVAL)
> + else
> + if mode ~= 'aggressive' then
> + log.info('Start aggressive discovery, %s buckets are '..
> + 'unknown. Discovery works with %s seconds '..
> + 'interval', unknown_bucket_count,
> + consts.DISCOVERY_WORK_INTERVAL)
> + mode = 'aggressive'
> + end
> + lfiber.sleep(consts.DISCOVERY_WORK_INTERVAL)
> + break
> + end
> + while M.errinj.ERRINJ_LONG_DISCOVERY do
> + M.errinj.ERRINJ_LONG_DISCOVERY = 'waiting'
> + lfiber.sleep(0.01)
> + end
> + until next(router.replicasets)
> end
> end
>
> @@ -355,9 +435,9 @@ local function discovery_set(router, new_mode)
> router.discovery_mode = new_mode
> if router.discovery_fiber ~= nil then
> pcall(router.discovery_fiber.cancel, router.discovery_fiber)
> + router.discovery_fiber = nil
> end
> if new_mode == 'off' then
> - router.discovery_fiber = nil
> return
> end
> router.discovery_fiber = util.reloadable_fiber_create(
> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
> index 73c6740..c6a78fe 100644
> --- a/vshard/storage/init.lua
> +++ b/vshard/storage/init.lua
> @@ -75,6 +75,7 @@ if not M then
> ERRINJ_RECEIVE_PARTIALLY = false,
> ERRINJ_NO_RECOVERY = false,
> ERRINJ_UPGRADE = false,
> + ERRINJ_DISCOVERY = false,
> },
> -- This counter is used to restart background fibers with
> -- new reloaded code.
> @@ -1110,10 +1111,58 @@ local function bucket_collect(bucket_id)
> return data
> end
>
> +-- Discovery used by routers. It returns limited number of
> +-- buckets to avoid stalls when _bucket is huge.
> +local function buckets_discovery_extended(opts)
> + local limit = consts.BUCKET_CHUNK_SIZE
> + local buckets = table.new(limit, 0)
> + local active = consts.BUCKET.ACTIVE
> + local pinned = consts.BUCKET.PINNED
> + local next_from
> + local errcnt = M.errinj.ERRINJ_DISCOVERY
> + if errcnt then
> + if errcnt > 0 then
> + M.errinj.ERRINJ_DISCOVERY = errcnt - 1
> + if errcnt % 2 == 0 then
> + box.error(box.error.INJECTION, 'discovery')
> + end
> + else
> + M.errinj.ERRINJ_DISCOVERY = false
> + end
> + end
> + -- No way to select by {status, id}, because there are two
> + -- statuses to select. A router would need to maintain a
> + -- separate iterator for each status it wants to get. This may
> + -- be implemented in future. But _bucket space anyway 99% of
> + -- time contains only active and pinned buckets. So there is
> + -- no big benefit in optimizing that. Perhaps a compound index
> + -- {status, id} could help too.
> + for _, bucket in box.space._bucket:pairs({opts.from},
> + {iterator = box.index.GE}) do
> + local status = bucket.status
> + if status == active or status == pinned then
> + table.insert(buckets, bucket.id)
> + end
> + limit = limit - 1
> + if limit == 0 then
> + next_from = bucket.id + 1
> + break
> + end
> + end
> + -- Buckets list can even be empty, if all buckets in the
> + -- scanned chunk are not active/pinned. But next_from still
> + -- should be returned. So as the router could request more.
> + return {buckets = buckets, next_from = next_from}
> +end
> +
> --
> -- Collect array of active bucket identifiers for discovery.
> --
> -local function buckets_discovery()
> +local function buckets_discovery(opts)
> + if opts then
> + -- Private method. Is not documented intentionally.
> + return buckets_discovery_extended(opts)
> + end
> local ret = {}
> local status = box.space._bucket.index.status
> for _, bucket in status:pairs({consts.BUCKET.ACTIVE}) do
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster
2020-05-04 14:26 ` Oleg Babin
@ 2020-05-04 21:09 ` Vladislav Shpilevoy
2020-05-06 8:27 ` Oleg Babin
0 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-04 21:09 UTC (permalink / raw)
To: Oleg Babin, tarantool-patches
Hi! Thanks for the review!
>>>> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
>>>> index 6d88153..26ea85b 100644
>>>> --- a/vshard/router/init.lua
>>>> +++ b/vshard/router/init.lua
>>>> @@ -250,50 +250,127 @@ if util.version_is_at_least(1, 10, 0) then
>>>> + -- Don't spam many requests at once. Give
>>>> + -- storages time to handle them and other
>>>> + -- requests.
>>>> + lfiber.sleep(consts.DISCOVERY_WORK_STEP)
>>>> + if module_version ~= M.module_version then
>>>> + return
>>>> end
>>>
>>>
>>> Is it possible to place such checks to some "common" places. At start/end of each iteration or between steps. This looks strange and a bit unobvous to do such checks after each timeout.
>>
>> Purpose is to stop the fiber as soon as possible in case a reload
>> happened. So moving it to a common place is not really possible.
>> It would make it work longer with the old code after reload. But
>> if we keep the checks, I still need to do then after/before any
>> long yield.
>>
>
> My main concern is that such things could be easily missed in future. But I agree that we should left this function early as possible.
It can be, and it was a couple of times. Don't know what to do with that
honestly, at this moment. This restart-on-reload machinery should be reworked
completely.
>> During working on your comments I found that during aggressive discovery I
>> added sleep in a wrong place. Fixed below:
>>
>> diff --git a/vshard/consts.lua b/vshard/consts.lua
>> index 5391c0f..a6a8c1b 100644
>> --- a/vshard/consts.lua
>> +++ b/vshard/consts.lua
>> @@ -40,5 +40,8 @@ return {
>> DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL = 0.5;
>> RECOVERY_INTERVAL = 5;
>> COLLECT_LUA_GARBAGE_INTERVAL = 100;
>> - DISCOVERY_INTERVAL = 10;
>> + DISCOVERY_IDLE_INTERVAL = 10,
>> + DISCOVERY_WORK_INTERVAL = 1,
>> + DISCOVERY_WORK_STEP = 0.01,
>> + DISCOVERY_TIMEOUT = 10,
>> }
>
> Nit: here commas and semicolons are mixed. I think we should support it in consistent state - only commas or only semicolons.
I don't like these ';'. They are against our code style. They were
added in the second commit in this repository, and since then kept
growing. I deliberately stopped using them and decided to switch to
',' in all new code, and wash out the old lines gradually.
I guess I can split DISCOVERY_* parameters with an empty line from the others
who still use ';' though. To make it more clear.
====================
diff --git a/vshard/consts.lua b/vshard/consts.lua
index a6a8c1b..8c2a8b0 100644
--- a/vshard/consts.lua
+++ b/vshard/consts.lua
@@ -40,6 +40,7 @@ return {
DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL = 0.5;
RECOVERY_INTERVAL = 5;
COLLECT_LUA_GARBAGE_INTERVAL = 100;
+
DISCOVERY_IDLE_INTERVAL = 10,
DISCOVERY_WORK_INTERVAL = 1,
DISCOVERY_WORK_STEP = 0.01,
====================
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster
2020-05-04 21:09 ` Vladislav Shpilevoy
@ 2020-05-06 8:27 ` Oleg Babin
0 siblings, 0 replies; 29+ messages in thread
From: Oleg Babin @ 2020-05-06 8:27 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Thanks for your answers.
On 05/05/2020 00:09, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!
>
>>>>> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
>>>>> index 6d88153..26ea85b 100644
>>>>> --- a/vshard/router/init.lua
>>>>> +++ b/vshard/router/init.lua
>>>>> @@ -250,50 +250,127 @@ if util.version_is_at_least(1, 10, 0) then
>>>>> + -- Don't spam many requests at once. Give
>>>>> + -- storages time to handle them and other
>>>>> + -- requests.
>>>>> + lfiber.sleep(consts.DISCOVERY_WORK_STEP)
>>>>> + if module_version ~= M.module_version then
>>>>> + return
>>>>> end
>>>>
>>>>
>>>> Is it possible to place such checks to some "common" places. At start/end of each iteration or between steps. This looks strange and a bit unobvous to do such checks after each timeout.
>>>
>>> Purpose is to stop the fiber as soon as possible in case a reload
>>> happened. So moving it to a common place is not really possible.
>>> It would make it work longer with the old code after reload. But
>>> if we keep the checks, I still need to do then after/before any
>>> long yield.
>>>
>>
>> My main concern is that such things could be easily missed in future. But I agree that we should left this function early as possible.
>
> It can be, and it was a couple of times. Don't know what to do with that
> honestly, at this moment. This restart-on-reload machinery should be reworked
> completely.
>
I see two ways:
* An external fiber that periodically checks module reload and reload
needed fibers.
* package.preload hook that does the same. (However I not completely
sure that it's possible)
But anyway I agree it's a separate task and such comments shouldn't
affect this patchset.
>>> During working on your comments I found that during aggressive discovery I
>>> added sleep in a wrong place. Fixed below:
>>>
>>> diff --git a/vshard/consts.lua b/vshard/consts.lua
>>> index 5391c0f..a6a8c1b 100644
>>> --- a/vshard/consts.lua
>>> +++ b/vshard/consts.lua
>>> @@ -40,5 +40,8 @@ return {
>>> DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL = 0.5;
>>> RECOVERY_INTERVAL = 5;
>>> COLLECT_LUA_GARBAGE_INTERVAL = 100;
>>> - DISCOVERY_INTERVAL = 10;
>>> + DISCOVERY_IDLE_INTERVAL = 10,
>>> + DISCOVERY_WORK_INTERVAL = 1,
>>> + DISCOVERY_WORK_STEP = 0.01,
>>> + DISCOVERY_TIMEOUT = 10,
>>> }
>>
>> Nit: here commas and semicolons are mixed. I think we should support it in consistent state - only commas or only semicolons.
>
> I don't like these ';'. They are against our code style. They were
> added in the second commit in this repository, and since then kept
> growing. I deliberately stopped using them and decided to switch to
> ',' in all new code, and wash out the old lines gradually.
>
> I guess I can split DISCOVERY_* parameters with an empty line from the others
> who still use ';' though. To make it more clear.
>
> ====================
> diff --git a/vshard/consts.lua b/vshard/consts.lua
> index a6a8c1b..8c2a8b0 100644
> --- a/vshard/consts.lua
> +++ b/vshard/consts.lua
> @@ -40,6 +40,7 @@ return {
> DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL = 0.5;
> RECOVERY_INTERVAL = 5;
> COLLECT_LUA_GARBAGE_INTERVAL = 100;
> +
> DISCOVERY_IDLE_INTERVAL = 10,
> DISCOVERY_WORK_INTERVAL = 1,
> DISCOVERY_WORK_STEP = 0.01,
>
> ====================
Ok. I agree. So I don't have comments for this patchset anymore. Feel
free to push it or ask somebody for additional review :)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster Vladislav Shpilevoy
2020-05-01 17:01 ` Oleg Babin
@ 2020-05-07 22:45 ` Konstantin Osipov
2020-05-08 19:56 ` Vladislav Shpilevoy
1 sibling, 1 reply; 29+ messages in thread
From: Konstantin Osipov @ 2020-05-07 22:45 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/05/01 22:16]:
Why not use merkle trees to only fetch the changed subset?
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster
2020-05-07 22:45 ` Konstantin Osipov
@ 2020-05-08 19:56 ` Vladislav Shpilevoy
2020-05-09 7:37 ` Konstantin Osipov
0 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-08 19:56 UTC (permalink / raw)
To: Konstantin Osipov, tarantool-patches, olegrok
On 08/05/2020 00:45, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/05/01 22:16]:
>
> Why not use merkle trees to only fetch the changed subset?
This is what I found: https://en.wikipedia.org/wiki/Merkle_tree
"Every leaf node is labelled with the cryptographic hash of a data
block, and every non-leaf node is labelled with the cryptographic
hash in the labels of its child nodes. They can help ensure that
data blocks received from other peers in a peer-to-peer network
are received undamaged and unaltered, and even to check that the
other peers do not lie and send fake blocks."
Correct me if I found something wrong.
Firstly, hashes has nothing to do with that. Discovery fetches
bucket ids (in ranges, usually). And I still need to fetch bucket
ids. It can't dehash a value, received from the storage, into a
range of buckets.
Secondly, storage does not depend on router and can't keep a state
of every router on it. If you meant, that the storage should keep
something on it.
Thirdly, there is no in-place change, which would mean you just need
to fetch a new version of a bucket from the same storage. Change means
the bucket was moved to a different replicaset (in 99.99999% cases).
Deleted from one, and added on another. So you need to download it from
the new place.
Otherwise I probably didn't understand what you meant.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster
2020-05-08 19:56 ` Vladislav Shpilevoy
@ 2020-05-09 7:37 ` Konstantin Osipov
0 siblings, 0 replies; 29+ messages in thread
From: Konstantin Osipov @ 2020-05-09 7:37 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/05/08 22:57]:
Here's a good description of how a merkle tree can be used:
https://docs.datastax.com/en/ddac/doc/datastax_enterprise/dbArch/archAntiEntropyRepair.html
With a merkle tree, you will only need to transfer the tree itself
and then the ranges which have actually changed.
> On 08/05/2020 00:45, Konstantin Osipov wrote:
> > * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/05/01 22:16]:
> >
> > Why not use merkle trees to only fetch the changed subset?
>
> This is what I found: https://en.wikipedia.org/wiki/Merkle_tree
> "Every leaf node is labelled with the cryptographic hash of a data
> block, and every non-leaf node is labelled with the cryptographic
> hash in the labels of its child nodes. They can help ensure that
> data blocks received from other peers in a peer-to-peer network
> are received undamaged and unaltered, and even to check that the
> other peers do not lie and send fake blocks."
>
> Correct me if I found something wrong.
>
> Firstly, hashes has nothing to do with that. Discovery fetches
> bucket ids (in ranges, usually). And I still need to fetch bucket
> ids. It can't dehash a value, received from the storage, into a
> range of buckets.
>
> Secondly, storage does not depend on router and can't keep a state
> of every router on it. If you meant, that the storage should keep
> something on it.
>
> Thirdly, there is no in-place change, which would mean you just need
> to fetch a new version of a bucket from the same storage. Change means
> the bucket was moved to a different replicaset (in 99.99999% cases).
> Deleted from one, and added on another. So you need to download it from
> the new place.
>
> Otherwise I probably didn't understand what you meant.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Tarantool-patches] [PATCH vshard 7/7] router: introduce discovery mode 'once'
2020-05-01 0:16 [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy
` (5 preceding siblings ...)
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster Vladislav Shpilevoy
@ 2020-05-01 0:16 ` Vladislav Shpilevoy
2020-05-01 17:02 ` Oleg Babin
2020-05-06 20:54 ` [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy
7 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-01 0:16 UTC (permalink / raw)
To: tarantool-patches, olegrok
Closes #210
@TarantoolBot document
Title: vshard.router.discovery_set() and new config option
```Lua
vshard.router.discovery_set(mode)
```
Turns on/off the background discovery fiber used by the router to
find buckets.
When `mode` is `"on"`, the discovery fiber works all the lifetime
of the router. Even after all buckets are discovered, it will
still go to storages and download their buckets with some big
period. This is useful, if bucket topology changes often and
bucket count is not big. Router will keep its route table up to
date even when no requests are processed. This is the default
value.
When `mode` is `"off"`, discovery is disabled completely.
When `mode` is `"once"`, discovery will start, find locations of
all the buckets, and then the discovery fiber is terminated. This
is good for large bucket count and for rarely clusters, where
rebalancing happens rarely.
The method is good to enable/disable discovery after the router is
already started, but discovery is enabled by default. You may want
to never enable it even for a short time - then specify
`discovery_mode` option in the configuration. It takes the same
values as `vshard.router.discovery_set(mode)`.
You may decide to turn off discovery or make it 'once' if you have
many routers, or tons of buckets (hundreds of thousands and more),
and you see that the discovery process consumes notable CPU % on
routers and storages. In that case it may be wise to turn off
discovery when there is no rebalancing in the cluster. And turn it
on for new routers, as well as for all routers when rebalancing is
started.
---
test/router/router2.result | 80 +++++++++++++++++++++++++++++++++++-
test/router/router2.test.lua | 34 ++++++++++++++-
vshard/cfg.lua | 6 +--
vshard/router/init.lua | 15 ++++++-
4 files changed, 129 insertions(+), 6 deletions(-)
diff --git a/test/router/router2.result b/test/router/router2.result
index 556f749..0f93fc4 100644
--- a/test/router/router2.result
+++ b/test/router/router2.result
@@ -113,16 +113,94 @@ vshard.router.static.discovery_fiber:status()
| - suspended
| ...
-f1:status(), f2, f3:status(), f4:status(), f5, f6:status()
+cfg.discovery_mode = 'once'
+ | ---
+ | ...
+vshard.router.cfg(cfg)
+ | ---
+ | ...
+f7 = vshard.router.static.discovery_fiber
+ | ---
+ | ...
+vshard.router.static.discovery_fiber:status()
+ | ---
+ | - suspended
+ | ...
+
+f1:status(), f2, f3:status(), f4:status(), f5, f6:status(), f7:status()
| ---
| - dead
| - null
| - dead
| - dead
| - null
+ | - dead
| - suspended
| ...
+vshard.router.bootstrap()
+ | ---
+ | - true
+ | ...
+function continue_discovery() \
+ local res = vshard.router.info().bucket.unknown == 0 \
+ if not res then \
+ vshard.router.discovery_wakeup() \
+ end \
+ return res \
+end
+ | ---
+ | ...
+
+-- With 'on' discovery works infinitely.
+vshard.router._route_map_clear()
+ | ---
+ | ...
+vshard.router.discovery_set('on')
+ | ---
+ | ...
+test_run:wait_cond(continue_discovery)
+ | ---
+ | - true
+ | ...
+vshard.router.info().bucket.unknown
+ | ---
+ | - 0
+ | ...
+vshard.router.static.discovery_fiber:status()
+ | ---
+ | - suspended
+ | ...
+
+-- With 'once' discovery mode the discovery fiber deletes self
+-- after full discovery.
+vshard.router._route_map_clear()
+ | ---
+ | ...
+vshard.router.discovery_set('once')
+ | ---
+ | ...
+test_run:wait_cond(continue_discovery)
+ | ---
+ | - true
+ | ...
+vshard.router.info().bucket.unknown
+ | ---
+ | - 0
+ | ...
+vshard.router.static.discovery_fiber
+ | ---
+ | - null
+ | ...
+-- Second set won't do anything.
+vshard.router.discovery_set('once')
+ | ---
+ | ...
+vshard.router.static.discovery_fiber
+ | ---
+ | - null
+ | ...
+
_ = test_run:switch("default")
| ---
| ...
diff --git a/test/router/router2.test.lua b/test/router/router2.test.lua
index 33f4d3e..10f82fd 100644
--- a/test/router/router2.test.lua
+++ b/test/router/router2.test.lua
@@ -41,7 +41,39 @@ vshard.router.discovery_set('on')
f6 = vshard.router.static.discovery_fiber
vshard.router.static.discovery_fiber:status()
-f1:status(), f2, f3:status(), f4:status(), f5, f6:status()
+cfg.discovery_mode = 'once'
+vshard.router.cfg(cfg)
+f7 = vshard.router.static.discovery_fiber
+vshard.router.static.discovery_fiber:status()
+
+f1:status(), f2, f3:status(), f4:status(), f5, f6:status(), f7:status()
+
+vshard.router.bootstrap()
+function continue_discovery() \
+ local res = vshard.router.info().bucket.unknown == 0 \
+ if not res then \
+ vshard.router.discovery_wakeup() \
+ end \
+ return res \
+end
+
+-- With 'on' discovery works infinitely.
+vshard.router._route_map_clear()
+vshard.router.discovery_set('on')
+test_run:wait_cond(continue_discovery)
+vshard.router.info().bucket.unknown
+vshard.router.static.discovery_fiber:status()
+
+-- With 'once' discovery mode the discovery fiber deletes self
+-- after full discovery.
+vshard.router._route_map_clear()
+vshard.router.discovery_set('once')
+test_run:wait_cond(continue_discovery)
+vshard.router.info().bucket.unknown
+vshard.router.static.discovery_fiber
+-- Second set won't do anything.
+vshard.router.discovery_set('once')
+vshard.router.static.discovery_fiber
_ = test_run:switch("default")
_ = test_run:cmd("stop server router_1")
diff --git a/vshard/cfg.lua b/vshard/cfg.lua
index 8a3e812..1ef1899 100644
--- a/vshard/cfg.lua
+++ b/vshard/cfg.lua
@@ -152,8 +152,8 @@ local function cfg_check_weights(weights)
end
local function check_discovery_mode(value)
- if value ~= 'on' and value ~= 'off' then
- error("Expected 'on' or 'off' for discovery_mode")
+ if value ~= 'on' and value ~= 'off' and value ~= 'once' then
+ error("Expected 'on', 'off', or 'once' for discovery_mode")
end
end
@@ -262,7 +262,7 @@ local cfg_template = {
is_optional = true, default = consts.DEFAULT_FAILOVER_PING_TIMEOUT
},
discovery_mode = {
- type = 'string', name = 'Discovery mode: on, off',
+ type = 'string', name = 'Discovery mode: on, off, once',
is_optional = true, default = 'on', check = check_discovery_mode
},
}
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index 26ea85b..927a38e 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -249,7 +249,7 @@ if util.version_is_at_least(1, 10, 0) then
--
discovery_f = function(router)
local module_version = M.module_version
- assert(router.discovery_mode == 'on')
+ assert(router.discovery_mode == 'on' or router.discovery_mode == 'once')
local iterators = {}
local opts = {is_async = true}
local mode
@@ -342,6 +342,13 @@ discovery_f = function(router)
unknown_bucket_count =
router.total_bucket_count - router.known_bucket_count
if unknown_bucket_count == 0 then
+ if router.discovery_mode == 'once' then
+ log.info("Discovery mode is 'once', and all is "..
+ "discovered - shut down the discovery process")
+ router.discovery_fiber = nil
+ lfiber.self():cancel()
+ return
+ end
if mode ~= 'idle' then
log.info('Discovery enters idle mode, all buckets are '..
'known. Discovery works with %s seconds '..
@@ -437,6 +444,12 @@ local function discovery_set(router, new_mode)
if new_mode == 'off' then
return
end
+ if new_mode == 'once' and
+ router.total_bucket_count == router.known_bucket_count then
+ -- 'Once' discovery is supposed to stop working when all
+ -- is found. But it is the case already. So nothing to do.
+ return
+ end
router.discovery_fiber = util.reloadable_fiber_create(
'vshard.discovery.' .. router.name, M, 'discovery_f', router)
end
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 7/7] router: introduce discovery mode 'once'
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 7/7] router: introduce discovery mode 'once' Vladislav Shpilevoy
@ 2020-05-01 17:02 ` Oleg Babin
2020-05-02 20:12 ` Vladislav Shpilevoy
0 siblings, 1 reply; 29+ messages in thread
From: Oleg Babin @ 2020-05-01 17:02 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Hi! Thanks for the patch. See one nit below.
On 01/05/2020 03:16, Vladislav Shpilevoy wrote:
> Closes #210
>
> @@ -342,6 +342,13 @@ discovery_f = function(router)
> unknown_bucket_count =
> router.total_bucket_count - router.known_bucket_count
> if unknown_bucket_count == 0 then
> + if router.discovery_mode == 'once' then
> + log.info("Discovery mode is 'once', and all is "..
> + "discovered - shut down the discovery process")
> + router.discovery_fiber = nil
> + lfiber.self():cancel()
> + return
> + end
I'm not sure that self.cancel is a good approach. I think simple
"return" should be enough.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 7/7] router: introduce discovery mode 'once'
2020-05-01 17:02 ` Oleg Babin
@ 2020-05-02 20:12 ` Vladislav Shpilevoy
2020-05-04 14:27 ` Oleg Babin
0 siblings, 1 reply; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-02 20:12 UTC (permalink / raw)
To: Oleg Babin, tarantool-patches
Thanks for the review!
>> @@ -342,6 +342,13 @@ discovery_f = function(router)
>> unknown_bucket_count =
>> router.total_bucket_count - router.known_bucket_count
>> if unknown_bucket_count == 0 then
>> + if router.discovery_mode == 'once' then
>> + log.info("Discovery mode is 'once', and all is "..
>> + "discovered - shut down the discovery process")
>> + router.discovery_fiber = nil
>> + lfiber.self():cancel()
>> + return
>> + end
>
> I'm not sure that self.cancel is a good approach. I think simple "return" should be enough.
The discovery fiber is restarted if it returns or throws. The only
way to stop it is to cancel. See reloadable_fiber_main_loop() in
vshard/vshard/util.lua.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH vshard 7/7] router: introduce discovery mode 'once'
2020-05-02 20:12 ` Vladislav Shpilevoy
@ 2020-05-04 14:27 ` Oleg Babin
0 siblings, 0 replies; 29+ messages in thread
From: Oleg Babin @ 2020-05-04 14:27 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Got it. Thanks for your answer. LGTM.
On 02/05/2020 23:12, Vladislav Shpilevoy wrote:
> Thanks for the review!
>
>>> @@ -342,6 +342,13 @@ discovery_f = function(router)
>>> unknown_bucket_count =
>>> router.total_bucket_count - router.known_bucket_count
>>> if unknown_bucket_count == 0 then
>>> + if router.discovery_mode == 'once' then
>>> + log.info("Discovery mode is 'once', and all is "..
>>> + "discovered - shut down the discovery process")
>>> + router.discovery_fiber = nil
>>> + lfiber.self():cancel()
>>> + return
>>> + end
>>
>> I'm not sure that self.cancel is a good approach. I think simple "return" should be enough.
>
> The discovery fiber is restarted if it returns or throws. The only
> way to stop it is to cancel. See reloadable_fiber_main_loop() in
> vshard/vshard/util.lua.
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery
2020-05-01 0:16 [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy
` (6 preceding siblings ...)
2020-05-01 0:16 ` [Tarantool-patches] [PATCH vshard 7/7] router: introduce discovery mode 'once' Vladislav Shpilevoy
@ 2020-05-06 20:54 ` Vladislav Shpilevoy
7 siblings, 0 replies; 29+ messages in thread
From: Vladislav Shpilevoy @ 2020-05-06 20:54 UTC (permalink / raw)
To: tarantool-patches, olegrok
Hi! Thanks for the review!
Pushed to master.
^ permalink raw reply [flat|nested] 29+ messages in thread