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

[Nikita Pettik]

On 05 Feb 23:07, Vladislav Shpilevoy wrote:
> 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.

Okay, I don't mind this change:

diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
index ed121770d..0e365361b 100644
--- a/doc/rfc/1148-stacked-diagnostics.md
+++ b/doc/rfc/1148-stacked-diagnostics.md
@@ -159,14 +159,14 @@ Tarantool returns a last-set (diag::last) error as `cdata` object from central
 diagnostic area to Lua in case of error. User should be unable to modify it
 (since it is considered to be a bad practice - in fact object doesn't belong
 to user). On the other hand, user needs an ability to inspect a collected
-diagnostic information. Hence, let's extend the `box.error` API with a function
+diagnostic information. Hence, let's extend the error object API with a method
 which provides the way to get the previous error (reason): `:prev()` (and
 correspondingly `.prev` field).
 
 ```
--- Return a reason error object for given error
+-- Return a reason error object for given error object 'e'
 -- (when exists, nil otherwise).
-box.error.prev(error) == error.prev
+e:prev(error) == error.prev
 ```