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 3E7CE24EF7 for ; Mon, 9 Sep 2019 04:13:12 -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 8glo0e0SKgpV for ; Mon, 9 Sep 2019 04:13:12 -0400 (EDT) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id E987E22041 for ; Mon, 9 Sep 2019 04:13:11 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool References: <5eaffc5d92bd910865f3da937f8b2831f968c4b0.1567435674.git.kshcherbatov@tarantool.org> From: Kirill Shcherbatov Message-ID: <7b5287fa-c430-ca96-3b5b-0b879f28fba9@tarantool.org> Date: Mon, 9 Sep 2019 11:13:06 +0300 MIME-Version: 1.0 In-Reply-To: <5eaffc5d92bd910865f3da937f8b2831f968c4b0.1567435674.git.kshcherbatov@tarantool.org> 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, georgy@tarantool.org Cc: alexander.turenko@tarantool.org, Vladislav Shpilevoy Hi! 1. > Sorry, but I couldn't match these two calls: > box.error.new(box.error.CREATE_SPACE, "myspace", "just I can't") > and > box.error.new({code = user_code, reason = user_error_msg}) Currently the box.error.new() endpoint supports two usages: box.error.new(errcode, format_str, ) and box.error.new({code = errcode, reason = error_msg_str}) Tarantool distinguishes these two usages by argument type. In scope of the patch I extend the second interface with new parameters to construct more informative error object on the client side (in net.box module) box.error.new({code = errcode, reason = error_msg_str, , , }) 2. >> +``` >> + >> +Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, >> `:__serialize()` methods and uniform `.type`, `.message` and `.trace` >> fields. + >> + > What if we want to throw a table error - as vshard does. https://github.com/tarantool/vshard/blob/master/vshard/error.lua As fores I've understood, vshard has an own error representation same as box_error adapter that constructs vshard-compatible table error object. The backward compatibility wouldn't be broken with introducing a couple new fields in core error object. However, to have a profit with new stacked diagnostics functionality the box_error adapter should be updated correspondingly. This likely requires extending vshard error functionality. I'd better create a related ticket for vshard module, if you are agree with me. > Additionally,if we would like to talk about user define errors we should > consider an API to register user error classes. Also it could be worth to > uncover error code/name clashing causes (both raising and catching). At first, such problem should be discussed taking into account the following ticket: https://github.com/tarantool/tarantool/issues/4398 Perhaps, we should introduce a new system space _errors to define new error classes. This must work in replicating environment. Extending of iproto IPROTO_ERROR_V2 wouldn't be a problem: we just need to introduce a new key "type" to transfer error class. Guess, it needn't be implemented as a part of #1148, right? 3. > I don't see any valid purpose for svp* methods. Please explain the cases when > we need to rollback a diag area. Kostya had a strong opinion about an ability to reset a last-errors set. I am able to imagine a situation where this could be useful. Some APIs may set errors inside while this error is not usefull, because we've called such function to get the result only: 1) space_cache_find returns space with a given id, actualize the space cache or returns NULL and sets the error. Most of them user_find/user_by_id, user_find_by_name etc. has two interfaces: that sets the error and that doesn't. Perhaps, sometimes reseting the error set (understanding why it is necessary) is better than forgetting to set the error at all. Actually, I don't know, but I really like svp-like API. >> Let's introduce >> `diag_svp(diag) -> SVP`, +`diag_rollback_to_svp(diag, SVP)`. The >> implementation proposal for SVP structure is given below: + >> +``` >> + truncate >> + xxxxxxxxxxxxxxxxxxxxxxxx SVP >> +DIAG: +-------+ | >> + | err | | >> + +-------+----->+-------+ here >> + third prev | err | >> + +-------+------->+-------+ >> + second prev | err | >> + +-------+ >> + first >> +``` >> + >> +The diag_structure already has `struct error *next` pointer; > It's called `last' if I didn't mistaken . >> + >> +Let's introduce a similar pointer in an error structure to organize a >> reason list: +``` >> +struct error { >> + ... >> + struct error *prev; > I would prefer if you called it reason. I completely agree with you, I also like 'reason' name more, but box.error.new({code = ..., reason = ..}) already use this name in other meaning (error message). So I've decided to use prev everywhere to avoid a mess. 4. > What with err->log() method? Would we change error formatting and logging if > an error has a reason? We've decided do not change log method earlier. Moreover, we decided do not change :unpack() method significantly (because of third-party modules like vshard that has assumptions about their output format). 5. >> +-- Return a reason error object for given error >> +-- (when exists, nil otherwise) >> +box.error.prev(error) == error.prev >> +``` > You let out of scope standard lua "error()" call. Please take it into > consideration. I can't say nothing about error() call. It is not box error, it can't be extended. It should be deprecated when #4398 would be closed, right? 6. >> +Let's extend existent binary protocol with a new key IPROTO_ERROR_V2: >> +{ >> + // backward compatibility >> + IPROTO_ERROR: "the most recent error message", >> + // modern error message >> + IPROTO_ERROR_V2: { >> + { >> + // the most recent error object >> + "code": error_code_number, >> + "reason": error_reason_string, >> + "file": file_name_string, >> + "line": line_number, >> + }, >> + ... >> + { >> + // the oldest (reason) error object >> + }, >> + } >> +} Is this format acceptable for you? Kostya talked a bit about "deprecation plan". Maybe it worth to be discussed.