[Tarantool-patches] [PATCH 0/6] Extending error functionality

[lvasiliev]

On 28.03.2020 16:54, Alexander Turenko wrote:
> Thanks for summarization!
> 
> I'll add my two coins.
> 
> CCed all participants.
> 
> WBR, Alexander Turenko.
> 
> On Sat, Mar 28, 2020 at 02:11:46AM +0300, lvasiliev wrote:
>> Summary of discussion with Alexander Turenko, Vlad, Mons, Sergos and Kirill
>> Yukhin:
>> 1) Session parameters
>> Move negotiations to UPDATE (as for _vsession_settings).
> 
> To be clear: use UPDATE of _vsession_settings instead of the negotiation
> protocol. It is less intruisive. Also it looks logical to have all
> session settings in _vsession_settings.
> 
>> As further optimization session settings can be moved to auth message for to
>> remove an additional RTT
> 
> However I don't sure whether it is really worth, because we can send two
> packets in a batch (send the second w/o waiting for answer of the first
> one). A client should handle four cases: {success, failure} x {success,
> failure} in the case: it does not look difficult.
> 
>>
>> 2) Backtrace
>> Leave adding to the error only Lua backtrace (which is added to the error at
>> luaT_pusherror()).
>> We must have a per server option for enable/disable adding backtrace.
>> In addition a bactrace enable/disable adding can be forced by the creation
>> function parametr.
> 
> Ok.
> 
>> The backtrace should be combined with the message if tostring is
>> used.(@knazarov )
> 
> We didn't discuss it on our meeting and, to be honest, I unsure here.
> 
> AFAIU, Kostya want to see a backtrace in logs when an error is not
> catched anywhere (and maybe in some other cases?). We should ask for
> expected behaviour rather that for certain implementation.
> 
> We have __tostring(), which is used when casting to a string explicitly
> or implicitly via concatenation. I would expect just error message here.
> 
> We have __serialize() for repl. It is logical to have backtrace and
> other fields here.
> 
> Note: __serialize() returns an error message and on the first glance it
> seems that we should keep it here for the old way marshaling over the
> binary protocol. However, no, we'll handle errors in serializers
> explicitly and will not use __serialize().
> 
> What method is in use when an error is written to logs (when not
> catched, when diag_log() is used, when written by log.info() from Lua)?
> I think it should be __serialize(), but it is possible that it is not so
> now.
> 
> Hm. Maybe Kostya is about case like:
> 
>   | local res, err = my_func()
>   | if res == nil then
>   |     log.info('My message, reason: ' .. err)
>   |     return
>   | end
> 
> What we'll propose for this case? Something like so?
> 
>   | log.info('My message, reason: ' .. yaml.encode(err))
> 
> Need to think.I will copy it to https://github.com/tarantool/tarantool/issues/4398 
because Kostya doesn't read the mailing list.
> 
>> Notes:
>> To add a C bt is possible when error is created (for example) but unwind the
>> Lua stack in diag.c is difficult (reason: the lua functions like
>> lua_getstack, lua_getinfo are unavailable where)
> 
> To be clear: C stack trace itself maybe not so valuabe, when it is not
> intermixed with Lua frames information. The case is using of a trigger.
> So we agreed to not add them, at least for now.
> 
>>
>> 3) About returning(return box.error.new(...)) an error through IPROTO
>> Now the error is serialized to string for marshaling through connection,
>> which makes it difficult to add various error parameters.
>> In the first iteration format was changed to table. But after discussion was
>> decided to add a new msgpack type for the errors transmitting(by analogy
>> with MP_DECIMAL)
> 
> Note: The new mp_exp subtype should have extensible format. I propose to
> keep key-value pairs like in mp_map or even contain mp_map inside. Keys
> should be number constants, not strings (to shrink packet size).
> 
>> An intermediate type mp_error will be introduced for this purposes (reason:
>> it is impossible to  create an object of the ClientError class (or some
>> other classes) at the time of decoding)
> 
> Seems ok.
> 
> However I would note that rewriting all error into C is also possible
> way to solve this.
In my mind it depends on how it will be implemented on C. Now you 
propose to move some additional details from module to the base 
implementation with a decrease of flexibility. All new implementations 
of errors will affect the Base.
> 
>> Due to the need to use different serialization methods, depending on the
>> session settings, the serializator context (containing fickle options) was
>> added.
> 
> Seems ok. Maybe.
> 
> However I still have some doubt about dividing options to 'rarely
> changed' and 'often changed'. It looks highly dependent of use case and
> does not looks as property of an option.
> 
> And it seems that it is necessary only to save copying of ~60-80 bytes
> of `struct serializer` for each serialization process.
> 
> Should not this context allow to store any serializer option? Or maybe
> we should provide some common way to speed up options 'copying'. Or
> maybe this <100 bytes copy is not so heavy?
> 
> Dividing options to rarely/often changed feels as not general solution
> of the problem for me and that is why I complain.
> 
> Maybe we can use caching here? Let's calculate hashsum of per-call
> serializer options and store full set of serializer options in a
> hashmap. When global options are changed, this hashmap should be flushed
> or updated.
> 
> This looks as general solution: it works for any option, it speeds up
> (at least in theory) per-call options usage.
> 
> TL;DR: Idea is to cache full serializer options using per-call
> serializer options (subset of all options).
>
In my opinion it looks like 
https://refactoring.guru/design-patterns/flyweight. I think 
implementation with cache is overkill and overhead.
>>
>> 4) Payload
>> For storage the payload at struct error will be added ptr to data with data
>> size.
>> For using a payload from Lua set/get_payload (worked with table as
>> argument/return type) will be used. It is possible to use msgpack encode
>> when set and decode on get, but not is rationally.
>> To reduce overhead is offered to save a link to the table and use it as
>> response output.
>> For these purposes is proposed to use inheritance from the struct error in
>> the C style with adding additional attributes like ptr to the Lua table. In
>> the case of a marshaling through net the payload will be encoded/decoded
>> with msgpack using.
> 
> Note: We should move all additional fields from error C++ classes to
> struct error (use union) and remove all C++ virtual methods from those
> classes. After this we can add more fields to a `struct error` child w/o
> padding.
> 
> Note: We should return a pointer to base (`struct error`) to Lua to keep
> cdata type being `struct error&`. It seems that iserror() is not
> documented and users likely use ffi.istype().
> 
> We agreed on serializing Lua payload into msgpack at encoding of an
> error (rather than doing it at an error creation). A table of virtual
> methods should be added so to serialize base error / lua error into
> msgpack (like it is done in `struct port`).
> 
> The reason to postpone serialization of payload to an error
> serialization is that we don't necessarily will serialize an error at
> all: maybe it'll be processed within a tarantool instance.
> 
I thought about this variant after discussion.
Now we have a structured, readable, self-documented implementation of 
the error hierarchy. If we will using  the union, it will look like ugly 
"all in one" (struct in union will be splitting in the case of adding a 
new type of error) and will be unreadable without comments. In addition, 
this variant will be very fragile (multiple inheritance). What do you 
think about implementation with adding to struct error void pointer with 
enum type_of_payload (variation of your original propose)?
> WBR, Alexander Turenko.
>