[Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Feb 4 23:48:45 MSK 2020
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.
More information about the Tarantool-patches
mailing list