Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fix!

On 05/02/2020 07:16, Nikita Pettik wrote:
> On 04 Feb 21:48, Vladislav Shpilevoy wrote:
>> Thanks for the RFC.
>>
>> It still does not conform with the template, but ok. I see that
>> that ship has sailed already, some other RFCs also violate the
>> template.
>>
>> See 2 comments below.
>>
>>>> 10. Are we not going to allow to link two existing errors? I imagine
>>>> it could be simpler and more flexible for a user, than filling
>>>> one big map in error.new().
>>>
>>> Okay, I'm not against it:
>>>
>>>  Another way to resolve this issue is to erase diagnostic area before
>>> @@ -173,13 +158,23 @@ box.error.prev(error) == error.prev
>>>  ```
>>>  
>>>  Furthermore, let's extend signature of `box.error.new()` with new (optional)
>>> -argument - the 'reason' parent error object:
>>> +argument - 'prev' - previous error object:
>>>  
>>>  ```
>>>  e1 = box.error.new({code = 111, reason = "just cause"})
>>>  e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
>>>  ```
>>>  
>>> +User may want to link already existing errors. To achieve this let's add
>>> +`set_prev` method to error object or/and `link` to `box.error` so that one can
>>> +join two errors:
>>> +```
>>> +e1 = box.error.new({code = 111, reason = "just cause"})
>>> +e2 = box.error.new({code = 222, reason = "just cause x2"})
>>> +...
>>> +e2.set_prev(e1) -- e2.prev == e1
>>> +box.error.link(e1, e2) -- e2.prev == e1
>>> +```
>>
>> 1. I don't think we need to change box.error global API. It would be
>> enough to add new methods to error object. box.error.link() and
>> box.error.prev() look redundant. What is a case, when they should
>> be used instead of error object methods?
>>
>> box.error.new() and last() exist because there is no other way to
>> create an error, or to get a last one.
> 
> I've added both since was not sure which one is better.
> Since you'd prefer avoid changing global interface (which is
> reasonable argument) let's leave only e:set_prev():
> 
> diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
> index d57e040ba..ed121770d 100644
> --- a/doc/rfc/1148-stacked-diagnostics.md
> +++ b/doc/rfc/1148-stacked-diagnostics.md
> @@ -178,14 +178,12 @@ e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
>  ```
>  
>  User may want to link already existing errors. To achieve this let's add
> -`set_prev` method to error object or/and `link` to `box.error` so that one can
> -join two errors:
> +`set_prev` method to error object so that one can join two errors:
>  ```
>  e1 = box.error.new({code = 111, reason = "just cause"})
>  e2 = box.error.new({code = 222, reason = "just cause x2"})
>  ...
>  e2.set_prev(e1) -- e2.prev == e1
> -box.error.link(e1, e2) -- e2.prev == e1
>  ```
>  ### Binary protocol

What about box.error.prev()? I don't think we need this one as
well. How will it work anyway? You just call box.error.prev()
multiple times and iterate over the error list? Or it can only
return the second error in the stack. Both ways looks not really
useful. Probably, error:prev() is enough.