[tarantool-patches] Re: [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool

[Kirill Shcherbatov]

Hi!

1.
> Sorry, but I couldn't match these two calls:
> box.error.new(box.error.CREATE_SPACE, "myspace", "just I can't")
> and
> box.error.new({code = user_code, reason = user_error_msg})

Currently the box.error.new() endpoint supports two usages:
box.error.new(errcode, format_str, <params>)
and
box.error.new({code = errcode, reason = error_msg_str})

Tarantool distinguishes these two usages by argument type.

In scope of the patch I extend the second interface with new parameters to construct
more informative error object on the client side (in net.box module)
box.error.new({code = errcode, reason = error_msg_str,
<file = filename_str>, <line = linenumber>,
<prev = reason_error_object>})

2.
>> +```
>> +
>> +Error cdata object has `:unpack()`, `:raise()`, `:match(...)`,
>> `:__serialize()` methods and uniform `.type`, `.message` and `.trace`
>> fields. +
>> +
> What if we want to throw a table error - as vshard does.

https://github.com/tarantool/vshard/blob/master/vshard/error.lua
As fores I've understood, vshard has an own error representation same as box_error
adapter that constructs vshard-compatible table error object.
The backward compatibility wouldn't be broken with introducing a couple new fields in core error object.

However, to have a profit with new stacked diagnostics functionality the box_error adapter should be
updated correspondingly. This likely requires extending vshard error functionality. 
I'd better create a related ticket for vshard module, if you are agree with me.


> Additionally,if we would like to talk about user define errors we should 
> consider an API to register user error classes. Also it could be worth to 
> uncover error code/name clashing causes (both raising and catching).

At first, such problem should be discussed taking into account the following ticket:
https://github.com/tarantool/tarantool/issues/4398

Perhaps, we should introduce a new system space _errors to define new error classes.
This must work in replicating environment. Extending of iproto IPROTO_ERROR_V2 wouldn't
be a problem: we just need to introduce a new key "type" to transfer error class.
Guess, it needn't be implemented as a part of #1148, right?

3.
> I don't see any valid purpose for svp* methods. Please explain the cases when 
> we need to rollback a diag area.
Kostya had a strong opinion about an ability to reset a last-errors set.

I am able to imagine a situation where this could be useful.
Some APIs may set errors inside while this error is not usefull, because
we've called such function to get the result only:
1) space_cache_find returns space with a given id, actualize the space cache or
returns NULL and sets the error.

Most of them user_find/user_by_id, user_find_by_name etc. has two interfaces:
that sets the error and that doesn't. Perhaps, sometimes reseting the error set
(understanding why it is necessary) is better than forgetting to set the error at
all.

Actually, I don't know, but I really like svp-like API. 

>> Let's introduce
>> `diag_svp(diag) -> SVP`, +`diag_rollback_to_svp(diag, SVP)`. The
>> implementation proposal for SVP structure is given below: +
>> +```
>> +                      truncate
>> +      xxxxxxxxxxxxxxxxxxxxxxxx SVP
>> +DIAG: +-------+               |
>> +      | err   |               |
>> +      +-------+----->+-------+ here
>> +      third    prev  | err   |
>> +                     +-------+------->+-------+
>> +                      second  prev    | err   |
>> +                                      +-------+
>> +                                       first
>> +```
>> +
>> +The diag_structure already has `struct error *next` pointer;
> It's called `last' if I didn't mistaken .
>> +
>> +Let's introduce a similar pointer in an error structure to organize a
>> reason list: +```
>> +struct error {
>> +   ...
>> +   struct error *prev;
> I would prefer if you called it reason.

I completely agree with you, I also like 'reason' name more, but
box.error.new({code = ..., reason = ..}) already use this name in other
meaning (error message). So I've decided to use prev everywhere to avoid
a mess.

4.
> What with err->log() method? Would we change error formatting and logging if 
> an error has a reason?

We've decided do not change log method earlier.
Moreover, we decided do not change :unpack() method significantly (because of third-party
modules like vshard that has assumptions about their output format).

5.
>> +-- Return a reason error object for given error
>> +-- (when exists, nil otherwise)
>> +box.error.prev(error) == error.prev
>> +```
> You let out of scope standard lua "error()" call. Please take it into 
> consideration.
 
I can't say nothing about error() call. It is not box error, it can't be extended.
It should be deprecated when #4398 would be closed, right?

6.
>> +Let's extend existent binary protocol with a new key IPROTO_ERROR_V2:
>> +{
>> +        // backward compatibility
>> +        IPROTO_ERROR: "the most recent error message",
>> +        // modern error message
>> +        IPROTO_ERROR_V2: {
>> +                {
>> +                        // the most recent error object
>> +                        "code": error_code_number,
>> +                        "reason": error_reason_string,
>> +                        "file": file_name_string,
>> +                        "line": line_number,
>> +                },
>> +                ...
>> +                {
>> +                        // the oldest (reason) error object
>> +                },
>> +        }
>> +}

Is this format acceptable for you?
Kostya talked a bit about "deprecation plan". Maybe it worth to be discussed.