Re: [Tarantool-patches] [PATCH v2 5/5] iproto: Update error MsgPack encoding

[lvasiliev <lvasiliev@tarantool.org>]

On 16.04.2020 3:11, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
>>> 7. Why are these values in a enum? I thought we
>>> decided to use string types?
>> Now we compare a string with error type when encode and use number further. If we encode error type as string we still have to compare the strings when decoding to create the error of the desired type. But number takes up less space versus string  when transmitting over a network and makes it possible to use a switch when create an error after decoding (which looks much nicer).
> 
> Wait, so this is inside the MessagePack too? This is not a good
> idea really. I thought you just converted strings to enums and
> then enums to classes.
Yes. It's not inside the MessagePack. MP_ERROR decodes by
external handler as we discussed. It registered by
luamp_set_decode_extension().

> 
> First about 'less space'. These are error objects. It does not
> really matter if every one of them becomes +10 or even +20 bytes
> if we use string types instead of numbers. They are not critical
> in any aspect, not in perf nor in space.
> 
> Second about switch, 'which looks nicer'. Nicely formatted if-else
> sequence also look fine. This is subjective though. I won't argue.
> 
> Thirdly, about why I think string types are better. The problem is
> that I don't want us to document all the error types as numbers
> and support them forever. When we use strings, we easily can remove
> old errors, add new errors. When we use numbers, we
> 
>      1) will need to document what every number means. Strings are
>      self-documenting. OutOfMemory means out of memory, obviously.
>      And so on.
Ok.
> 
>      2) will one day have holes in these numbers left from removed
>      errors, this won't look nice, trust me.
Ok.
> 
>      3) that complicates compatibility. What if some error type was
>      added to a newer tarantool version, and an old connector connected
>      to the instance? How will it handle the new error types? With
>      string types the problem does not exist.
If I understand you correctly, you will have the same problem
with string, because you can't create an error of unknown type.
> 
> Numbers are fine for error codes. For example, SQL drivers define
> certain error codes as kind of a standard, and that simplifies
> support. Also an error code can describe actually a pretty big
> problem, which would be impossible to say in a short string. This
> is not so for types.
> 
> I hope you follow my idea, I don't want you to just blindly agree,
> if you actually don't agree and don't want to argue either.
I will update this in the future version of the patchset.
> 
>>>> 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.
>> fixed
>>
>> I have a some additional question:
>> "Do I understand correctly that MP_ERROR should not be added to field_def.h/field_def.c or I am wrong?"
> 
> I didn't read to that place, but sounds strange. We can't create a field
> of type 'error' in a space. We can't store errors anywhere. So it looks
> incorrect for field_def to depend on MP_ERROR.
> 
As I understand, you haven't reviewed this patch. Please see version
from patchset V3. I will apply all updates after the review, I assume
there will be questions and objections.