[tarantool-patches] Re: [PATCH 2/2] Fix discovery/reconfigure race

[Vladislav Shpilevoy]

Thanks for the fixes! See 2 comments below.

> commit bc90c3db5f5b1b663c747b8c2829fc8528af70cf
> Author: AKhatskevich <avkhatskevich at tarantool.org>
> Date:   Thu Jun 14 16:03:09 2018 +0300
> 
>      Fix discovery/reconfigure race
> 
>      This commit prevents discovery fiber from discovering old replicasets
>      and spoiling `route_map`.
> 
> diff --git a/test/router/router.result b/test/router/router.result
> index 5643f3e..36d54bf 100644
> --- a/test/router/router.result
> +++ b/test/router/router.result
> @@ -1095,6 +1095,55 @@ for bucket, old_rs in pairs(bucket_to_old_rs) do
>   end;
>   ---
>   ...
> +--
> +-- Check route_map is not filled with old replica objects after
> +-- reconfigure.
> +--
> +-- Simulate long `callro`.
> +vshard.router.internal.errinj.ERRINJ_LONG_DISCOVERY = true;
> +---
> +...
> +while not vshard.router.internal.errinj.ERRINJ_LONG_DISCOVERY == 'waiting' do

1. Why 'not value == needed_value'? Why not 'value ~= needed_value'? And how
does it work? I checked this thing in Lua and got these results:

-- Before cycle
tarantool> not true == 'waiting'
---
- false
...


-- After 'waiting' is set
tarantool> not 'waiting' == 'waiting'
---
- false
...


So this cycle is never run. And I proved it with putting assert(false)
into the first line - the test passed as well:

	[001] +++ router/router.reject	Wed Jun 27 14:39:30 2018
	[001] @@ -1104,6 +1104,7 @@
	[001]  ---
	[001]  ...
	[001]  while not vshard.router.internal.errinj.ERRINJ_LONG_DISCOVERY == 'waiting' do
	[001] +    assert(false)
	[001]      vshard.router.discovery_wakeup()
	[001]      fiber.sleep(0.02)
	[001]  end;
	[001]

Please, investigate why your test passes even without this cycle again.

> +    vshard.router.discovery_wakeup()
> +    fiber.sleep(0.02)
> +end;
> +---
> +...
> +vshard.router.cfg(cfg);
> +---
> +...
> +route_map = vshard.router.internal.route_map
> +for bucket_id, _ in pairs(route_map) do
> +    route_map[bucket_id] = nil
> +end;

2. Why not just 'vshard.router.internal.route_map = {}' ?