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

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

> I had the objection against a list of errors in the diagnostic area: a
> user may want to save an error, perform some new commands, which can
> rewrite a last diagnostic (replace the diagnostic list) and then (s)he
> may want to look at the stack of reasons of the saved error. But now it
> seems you discarded the idea. (BTW, it would be good to briefly mention
> changes you made for a new revision of your document: the text looks
> similar and it is hard to find differences.)

Not sure if I understood the problem. What is wrong the list of errors?
And how does it correlate with an error accidental removal? If I got
the problem right, it exists even now, and does not depend on structure
of an error. Even errno has that problem, and the only way to solve it -
save the error to a local variable, perform needed preparations, and look
at the saved value. You can do it now. Just save 'box.error.last()', do
your stuff, and deal with the saved value.

> 
> I think that struct error should have a pointer to its cause, which is
> another error.

But it does exactly this now. It is called 'reason', which IMO is
easier to understand. My first association with 'cause' is an informal
writing of 'because', and it confuses a bit.

> I like Java terminology here: cause is exactly what we want to save for
> an error. Wrap / unwrap (Go terminology) look too abstract and maybe
> even unclear for me. Maybe this terms appears from merry / xerror
> packages, which were inspired to wrap built-in errors with additional
> information (say, stack traces). It is up to maintainers, but my vote is
> for 'cause' field, 'get_cause()' and 'set_cause()' functions (maybe with
> some prefix, because it is C).

Strangely, but I understood wrap/unwrap easily. So I vote to keep them.
But if there are more people who doesn't understand, then here is my
humble proposal:

    C API:
    box_error_reason(const struct error *) // To get a reason error.
    box_error_set_reason(struct error *, struct error *) // To set a reason.

    Lua API:
    error:reason() -- Get a reason error.
    error:set_reason(reason) -- Set a reason error.

> 'Is' method (Go terminology, here it is named 'has') also don't look
> self-descriptive enough for me. I would name it 'find_cause()'.

I vote for just 'find'. I don't think 'cause' or 'reason' should be a
part of that function's name, because a real reason of the final result
is only one - the first error. This function on the contrary is able
to find any error in stack.

> 
> I doubt a bit that iproto change you proposed would be
> backward-compatible: imagine that you connect from an old tarantool to a
> new tarantool using net.box or handle errors using another existing
> connector. I would send those messages as before, but add a field with
> IPROTO_ERROR_LIST key in addition to existing IPROTO_ERROR (0x31) to a
> body map. The value can be a list of maps like {code = <mp_uint>,
> message = <mp_str>} or a list of list [<code>, <message>]. The
> IPROTO_ERROR_LIST should start from a last (top) error I think.

I agree. I forgot about backward compatibility. Indeed, we can send both
new single error, and a list of errors. The list can even contain the
last error second time, because anyway error packets are rare and not
normal. We may not optimize them too much in terms of space.

> 
> Maybe we should use 'warnings' term here if this feature is intended to
> be used for SQL warnings. If we want to use the proposed mechanics for
> warnings, then my proposal re using 'cause' term looks doubtful. Don't
> sure whether we should introduce some kind of warnings list for the
> diagnostic area or reuse 'cause' / 'parent' / ... field of struct error.
> 
> I didn't look at your patch: I want to agree on terms and overall
> approach first. At least one of maintainers should approve it.

Most of you objections are about names, so I guess you already can take
a look at the functionality and tests.