From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 80EAD46970E for ; Thu, 6 Feb 2020 20:24:15 +0300 (MSK) Date: Thu, 6 Feb 2020 20:24:14 +0300 From: Nikita Pettik Message-ID: <20200206172414.GA27490@tarantool.org> References: <3f49ddc14c2e07f4ac96fc71f3912fd0c5663882.1579032293.git.korablev@tarantool.org> <26c58292-4720-49b0-0770-6bafb559936f@tarantool.org> <20200127112337.GE1144@tarantool.org> <573176b9-9228-24b1-c825-1a13fbef4834@tarantool.org> <20200205061616.GG1049@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org 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 ```