[Tarantool-patches] [PATCH v2 5/5] iproto: Update error MsgPack encoding
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Apr 14 04:12:11 MSK 2020
Thanks for the patch!
Sorry, some of my comments may contain typos, because it is
very late and I can't continue polishing it today.
I didn't have time to review it properly yet. Just some
first comments below. See 12 of them below.
On 10/04/2020 10:10, Leonid Vasiliev wrote:
> New MsgPack encode format for error (MP_ERROR) has been added.
> If the extended error format is enabled using iproto session settings
> MP_ERROR type will be used for transferring error through network,
> MP_STR was used before.
1. For such a big feature and not a single word of documentation.
> Needed for #4398
2. Shouldn't that be 'Closes'?
> diff --git a/src/box/error.cc b/src/box/error.cc
> index 8179e52..f2e60c1 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -253,6 +253,13 @@ XlogGapError::XlogGapError(const char *file, unsigned line,
> (long long) vclock_sum(to), s_to ? s_to : "");
> }
>
> +XlogGapError::XlogGapError(const char *file, unsigned line,
> + const char *msg)
> + : XlogError(&type_XlogGapError, file, line)
> +{
> + error_format_msg(this, "%s", msg);
> +}
> +
> struct error *
> BuildXlogGapError(const char *file, unsigned line,
> const struct vclock *from, const struct vclock *to)
> @@ -264,6 +271,16 @@ BuildXlogGapError(const char *file, unsigned line,
> }
> }
>
> +struct error *
> +ReBuildXlogGapError(const char *file, unsigned line, const char *msg)
3. Wtf is Rebuild? Why normal Build does not work? If that is
because of vclock, then just change vclock to const char * in
the original Build in a separate preparatory commit.
> +{
> + try {
> + return new XlogGapError(file, line, msg);
> + } catch (OutOfMemory *e) {
> + return e;
> + }
> +}
> +
> struct rlist on_access_denied = RLIST_HEAD_INITIALIZER(on_access_denied);
> > diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index 5d3579e..6d1d247 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -174,7 +175,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
> */
> for (int i = 1; i <= nrets; ++i) {
> struct luaL_field field;
> - if (luaL_tofield(L, cfg, i, &field) < 0)
> + if (luaL_tofield(L, cfg, NULL, i, &field) < 0)
4. I think addition of that parameter should be a part of the previous
commit. So as not to pollute this commit with such minor changes.
In the previous commit it can be added and ignored inside luamp_encode_r().
But it will absorb all these not interesting changes.
> return luaT_error(L);
> struct tuple *tuple;
> if (field.type == MP_EXT &&
> @@ -379,6 +380,7 @@ execute_lua_eval(lua_State *L)
> struct encode_lua_ctx {
> struct port_lua *port;
> struct mpstream *stream;
> + struct luaL_serializer_ctx *serializer_ctx;
5. Not. A. Single. Word. Please, start writing descriptive
comments.
> };
>
> static int
> diff --git a/src/box/lua/mp_error.cc b/src/box/lua/mp_error.cc
> new file mode 100644
> index 0000000..f99da0f
> --- /dev/null
> +++ b/src/box/lua/mp_error.cc
> @@ -0,0 +1,454 @@
> +/*
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + * copyright notice, this list of conditions and the
> + * following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following
> + * disclaimer in the documentation and/or other materials
> + * provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +#include "box/lua/mp_error.h"
> +#include "box/error.h"
> +#include "mpstream.h"
> +#include "msgpuck.h"
> +#include "mp_extension_types.h"
> +
> +enum mp_error_details {
> + MP_ERROR_DET_TYPE,
> + MP_ERROR_DET_FILE,
> + MP_ERROR_DET_LINE,
> + MP_ERROR_DET_REASON,
> + MP_ERROR_DET_ERRNO,
> + MP_ERROR_DET_CODE,
> + MP_ERROR_DET_BACKTRACE,
> + MP_ERROR_DET_CUSTOM_TYPE,
> + MP_ERROR_DET_AD_OBJ_TYPE,
> + MP_ERROR_DET_AD_OBJ_NAME,
> + MP_ERROR_DET_AD_ACCESS_TYPE
> +};
> +
> +enum mp_error_types {
> + MP_ERROR_TYPE_UNKNOWN,
> + MP_ERROR_TYPE_CLIENT,
> + MP_ERROR_TYPE_CUSTOM,
> + MP_ERROR_TYPE_ACCESS_DENIED,
> + MP_ERROR_TYPE_XLOG,
> + MP_ERROR_TYPE_XLOG_GAP,
> + MP_ERROR_TyPE_SYSTEM,
6. I assume you use some kind of smart editor with
autocompletion so as you missed this typo:
TyPE -> TYPE
7. Why are these values in a enum? I thought we
decided to use string types?
> + MP_ERROR_TyPE_SOCKET,
> + MP_ERROR_TyPE_OOM,
> + MP_ERROR_TyPE_TIMED_OUT,
> + MP_ERROR_TyPE_CHANNEL_IS_CLOSED,
> + MP_ERROR_TyPE_FIBER_IS_CANCELLED,
> + MP_ERROR_TyPE_LUAJIT,
> + MP_ERROR_TyPE_ILLEGAL_PARAMS,
> + MP_ERROR_TyPE_COLLATION,
> + MP_ERROR_TyPE_SWIM,
> + MP_ERROR_TyPE_CRYPTO
> +};
> +
> +struct mp_error {
> + uint32_t error_code;
> + uint8_t error_type;
8. If you assign enum to a variable, that variable should
have type of that enum, usually.
> + uint32_t line;
> + uint32_t saved_errno;
> + char *file;
> + char *backtrace;
> + char *reason;
> + char *custom_type;
> + char *ad_obj_type;
> + char *ad_obj_name;
> + char *ad_access_type;
> +};
> diff --git a/src/box/lua/mp_error.h b/src/box/lua/mp_error.h
> new file mode 100644
> index 0000000..9eab213
> --- /dev/null
> +++ b/src/box/lua/mp_error.h
> @@ -0,0 +1,49 @@
> +#pragma once
> +/*
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + * copyright notice, this list of conditions and the
> + * following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following
> + * disclaimer in the documentation and/or other materials
> + * provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +#include "stdint.h"
9. Standard and other external headers should be in <>, not "".
> +
> +struct mpstream;
> +
> +void
> +error_to_mpstream(struct error *error, struct mpstream *stream);
> +
> +struct error *
> +error_unpack(const char **data, uint32_t len);
> +
> +#if defined(__cplusplus)
> +} /* extern "C" */
> +#endif /* defined(__cplusplus) */
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index c8f76b0..d3997af 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -1050,7 +1050,7 @@ local function new_sm(host, port, opts, connection, greeting)
>
> -- Set extended error format for session.
> if opts.error_extended then
> - local ext_err_supported = version_at_least(remote.peer_version_id, 2, 4, 1)
> + local ext_err_supported = version_at_least(remote.peer_version_id, 2, 4, 0)
10. Why?
> if not ext_err_supported then
> box.error(box.error.PROC_LUA,
> "Server doesn't support extended error format")
> diff --git a/src/lib/core/mp_extension_types.h b/src/lib/core/mp_extension_types.h
> index bc9873f..a2a5079 100644
> --- a/src/lib/core/mp_extension_types.h
> +++ b/src/lib/core/mp_extension_types.h
> @@ -40,8 +40,9 @@
> * You may assign values in range [0, 127]
> */
> enum mp_extension_type {
> - MP_UNKNOWN_EXTENSION = 0,
> - MP_DECIMAL = 1,
> + MP_UNKNOWN_EXTENSION = 0,
> + MP_DECIMAL = 1,
> + MP_ERROR = 2
> };
11. Please, rebase before applying any review fixes. This code is
changed in the master. As well as session settings.
> #endif
> diff --git a/src/lua/error.c b/src/lua/error.c
> index cd6ab54..109f947 100644
> --- a/src/lua/error.c
> +++ b/src/lua/error.c
> @@ -34,8 +34,6 @@
> #include <fiber.h>
> #include "utils.h"
>
> -static int CTID_CONST_STRUCT_ERROR_REF = 0;
> -
> struct error *
> luaL_iserror(struct lua_State *L, int narg)
> {
> diff --git a/src/lua/error.h b/src/lua/error.h
> index 16cdaf7..4e4dc04 100644
> --- a/src/lua/error.h
> +++ b/src/lua/error.h
> @@ -37,7 +37,6 @@
> extern "C" {
> #endif /* defined(__cplusplus) */
>
> -
12. Please, remove all unnecessary diff.
More information about the Tarantool-patches
mailing list