[tarantool-patches] Re: [PATCH 1/2] Add test on error during reconfigure

[Vladislav Shpilevoy]

Hello. Thanks for the fixes!

On 18/06/2018 13:10, Alex Khatskevich wrote:
> 
>>> +-- Error during reconfigure process.
>>> +_ = vshard.router.route(1):callro('echo', {'some_data'})
>>
>> 1. Why do you need this call here? It does nor outputs
>> nothing.
> Because next call checks a raised error.

callro never raises (except OOM and usage error). So if here
an error occurs, you will not see it. Same in other places below.

> This line ensures that an error ocured only after
> `vshard.router.internal.errinj.ERRINJ_CFG = true`
> just by not failing.
> 
> I did it to prevent test.result flooding.
>>> +---
>>> +...
>>> +vshard.router.internal.errinj.ERRINJ_CFG = true
>>> +---
>>> +...
>>> +old_internal = table.copy(vshard.router.internal)
>>> +---
>>> +...
>>> +_, err = pcall(vshard.router.cfg, cfg)
>>> +---
>> 2. Same. Maybe this call should output something? This
>> test would not fail if callro failed. Call/ro/rw do not
>> throw exceptions but return a pair: result and error if
>> occurred.
> The error I test here returns as a raised exception.
>> 3. Same. Here and at the end of the test.
> Same
>>> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
>>> index 21093e5..1dee80c 100644
>>> --- a/vshard/router/init.lua
>>> +++ b/vshard/router/init.lua
>>> @@ -473,6 +474,12 @@ local function router_cfg(cfg)
>>>       end
>>>       box.cfg(cfg)
>>>       log.info("Box has been configured")
>>> +    -- It is considered that all possible errors during cfg
>>> +    -- process occur only before this place.
>>> +    -- This check should be placed as late as possible.
>>> +    if M.errinj.ERRINJ_CFG then
>>> +        error('Error injection: cfg')
>>> +    end
>>
>> 4. I think, you should place this injection before box.cfg.
>> All the cfg() code relies on the fact, that box.cfg is atomic.
>> And if box.cfg is ok, then storage/router.cfg finish with
>> no errors. An error is possible only before box.cfg and in
>> box.cfg. You may think box.cfg like commit, and code before
>> box.cfg like prepare.
> In this case it does not matter, because it is just an errinj. However I wanted to show here,
> that error should be raised as late as possible. It can help in the future, if someone adds a code after `box.cfg`. I prefer errnj to be here and will move it only if you insist.

I understand, but any error is impossible here after box.cfg{}. Please, move this
injection one line up.