From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B1E3F4696C3 for ; Thu, 16 Apr 2020 03:12:02 +0300 (MSK) References: <767fd7dee482d61cda4e2c7c9e938628bbd7f84d.1586505741.git.lvasiliev@tarantool.org> <38ee9237-e6a2-24a5-b418-8ca05d5e16e1@tarantool.org> From: Vladislav Shpilevoy Message-ID: <94b59a28-8387-3b7a-8667-f92cd8dbfc18@tarantool.org> Date: Thu, 16 Apr 2020 02:11:59 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v2 5/5] iproto: Update error MsgPack encoding List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: lvasiliev Cc: tarantool-patches@dev.tarantool.org 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.