From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 84B4646970E for ; Tue, 4 Feb 2020 23:48:47 +0300 (MSK) References: <3f49ddc14c2e07f4ac96fc71f3912fd0c5663882.1579032293.git.korablev@tarantool.org> <26c58292-4720-49b0-0770-6bafb559936f@tarantool.org> <20200127112337.GE1144@tarantool.org> From: Vladislav Shpilevoy Message-ID: <573176b9-9228-24b1-c825-1a13fbef4834@tarantool.org> Date: Tue, 4 Feb 2020 21:48:45 +0100 MIME-Version: 1.0 In-Reply-To: <20200127112337.GE1144@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org 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.