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

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

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.

>>> +### Binary protocol
>>> +
>>> +Currently errors are sent as `(IPROTO_ERROR | errcode)` response with an
>>> +string message containing error details as a payload. There are not so many
>>> +options to extend current protocol wihtout breaking backward compatibility
>>> +(which is obviously one of implementation requirements). One way is to extend
>>> +existent binary protocol with a new key IPROTO_ERROR_STACK (or
>>> +IPROTO_ERROR_REASON or simply IPROTO_ERROR_V2):
>>> +```
>>> +{
>>> +        // backward compatibility
>>> +        IPROTO_ERROR: "the most recent error message",
>>> +        // modern error message
>>> +        IPROTO_ERROR_STACK: {
>>> +                {
>>> +                        // the most recent error object
>>> +                        IPROTO_ERROR_CODE: error_code_number,
>>> +                        IPROTO_ERROR_REASON: error_reason_string,
>>> +                },
>>> +                ...
>>> +                {
>>> +                        // the oldest (reason) error object
>>> +                },
>>> +        }
>>> +}
>>> +```
>>
>> 11. The names look good. Except that maybe change IPROTO_ERROR_REASON ->
>> IPROTO_ERROR_MESSAGE. I don't know why it is called reason everywhere
>> in our public API. In struct error it is message, not reason. And it
>> just looks more correct, IMO.
> 
> On the other hand, it might look a bit inconsistent if in binary protocole
> field would have different name from one in Lua API. Nevertheless, I
> don't mind this renaming. Fixed in RFC.
> 
>> 12. Another option - send multiple IPROTO_ERROR's. In MP_MAP you can
>> put multiple keys with the same value, and even keep a strict order.
>> So we could send IPROTO_ERROR: ..., IPROTO_ERROR: ..., IPROTO_ERROR: ...
>> For each message in the stack. But that looks like an illegal hack,
>> and also does not allow to fix this thing alongside:
>> https://github.com/tarantool/tarantool/issues/3399
>>
>> So a proper stack looks better, like you proposed.
>>
>>> +
>>> +IPROTO_ERROR is always sent (as in older versions) in case of error.
>>> +IPROTO_ERROR_STACK is presented in response only if there's at least two
>>> +elements in diagnostic list. Map which contains error stack can be optimized
>>> +in terms of space, so that avoid including error which is already encoded
>>> +in IPROTO_ERROR.
>>>
>>
>> 13. I don't think it is worth saving a few bytes for the cold path of error
>> reporting. Lets always send IPROTO_ERROR with a last error like now *and*
>> IPROTO_ERROR_STACK. Even if they contain the same error. It will make much
>> easier to remove IPROTO_ERROR later. Otherwise we will need to carry them
>> both forever, if IPROTO_ERROR_STACK would depend on IPROTO_ERROR.
> 
> Ok (it is only suggestion in scope of RFC, so I guess your comment doesn't
> imply that last remark concerning optimization should be erased).

2. The comments and alternatives can be kept, yes.