[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