From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 41D7326A79 for ; Thu, 8 Aug 2019 16:44:03 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ydsyi3o-dI9P for ; Thu, 8 Aug 2019 16:44:03 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 7C16A21310 for ; Thu, 8 Aug 2019 16:44:02 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool References: <101e2b28c29ebfc4cf58b45e534207fd4f6d9b3a.1564657285.git.kshcherbatov@tarantool.org> <20190807232755.s7gruhxeja75hob7@tkn_work_nb> From: Vladislav Shpilevoy Message-ID: Date: Thu, 8 Aug 2019 22:46:29 +0200 MIME-Version: 1.0 In-Reply-To: <20190807232755.s7gruhxeja75hob7@tkn_work_nb> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Alexander Turenko , Kirill Shcherbatov Cc: kostja@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 = , > message = } or a list of list [, ]. 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.