From: Kirill Shcherbatov <kshcherbatov@tarantool.org> To: tarantool-patches@freelists.org, georgy@tarantool.org Cc: alexander.turenko@tarantool.org, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool Date: Mon, 9 Sep 2019 11:13:06 +0300 [thread overview] Message-ID: <7b5287fa-c430-ca96-3b5b-0b879f28fba9@tarantool.org> (raw) In-Reply-To: <5eaffc5d92bd910865f3da937f8b2831f968c4b0.1567435674.git.kshcherbatov@tarantool.org> 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, <params>) 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, <file = filename_str>, <line = linenumber>, <prev = reason_error_object>}) 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.
next prev parent reply other threads:[~2019-09-09 8:13 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-09-02 14:51 [tarantool-patches] [PATCH v3 0/6] box: stacked diagnostics area in fiber Kirill Shcherbatov 2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov 2019-09-09 8:13 ` Kirill Shcherbatov [this message] 2019-09-09 19:44 ` [tarantool-patches] " Vladislav Shpilevoy 2019-09-09 19:44 ` Vladislav Shpilevoy 2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 2/6] box: rename diag_add_error to diag_set_error Kirill Shcherbatov 2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 3/6] box: introduce stacked diagnostic area Kirill Shcherbatov 2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 4/6] box: extend box.error.new({}) API Kirill Shcherbatov 2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 5/6] iproto: refactor error encoding with mpstream Kirill Shcherbatov 2019-09-04 10:53 ` [tarantool-patches] " Konstantin Osipov 2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 6/6] iproto: support stacked diagnostics for errors Kirill Shcherbatov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=7b5287fa-c430-ca96-3b5b-0b879f28fba9@tarantool.org \ --to=kshcherbatov@tarantool.org \ --cc=alexander.turenko@tarantool.org \ --cc=georgy@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox