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

[Vladislav Shpilevoy]

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.

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.

    2) will one day have holes in these numbers left from removed
    errors, this won't look nice, trust me.

    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.

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.

>>> 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.