From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 4A9CB4696C3 for ; Sat, 4 Apr 2020 02:17:51 +0300 (MSK) References: <20200401160914.GC26447@tarantool.org> <50ab4dbe-0e2e-9d65-6087-4d01feb94b8e@tarantool.org> <20200402174232.GA31861@tarantool.org> <24e2df72-400f-11f9-25a2-eabf871e6cb5@tarantool.org> <20200403015416.GB946@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sat, 4 Apr 2020 01:17:49 +0200 MIME-Version: 1.0 In-Reply-To: <20200403015416.GB946@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the fixes! On 03/04/2020 03:54, Nikita Pettik wrote: > On 03 Apr 00:20, Vladislav Shpilevoy wrote: >> Hi! Thanks for the fixes! >> >>>> Also a small comment about cycle detection in your patch. Here I >>>> would strongly-strongly recommend to avoid it when it is clearly >>>> not necessary. >>> >>> To be honest, I don't understand why do you care about this opimization so >>> much. Linking errors into list is unlikely to be hot execution path, so nobody >>> should bother with such minor improvement here. >> >> I am really afraid, that our solution team will find a way to >> shoot in their legs. For example, by doing some weird banking >> application producing error stacks with 50 errors and more, or >> they will add many errors when will forward them in the network >> from node to node (they actually want this, and are going to do >> that when we introduce custom errors), and will waste notable >> time on cycle checking. And probably will even hit the cycle >> error, and will complain. This is what I am afraid of. I am >> trying to save every CPU cycle. >> >>> Instead, keeping code clear, >>> simple and straightforward (your diff contains more branches and looks a bit >>> more complicated to me) must be the main goal. Applied diff below only due to >>> your recommendation. >>> >>> >>> Still, I haven't changed my mind. I didn't apply diff below, sorry. >>> I'll leave it to the second reviewer. >> >> Not sure there will be a second reviewer. From what I understand, the >> release is begin of next week. I am ok, lets leave it then as is. > > There's still 56 open issues assigned to 2.4 milestone. I doubt > that all of them will be closed till Monday... Didn't know that. In this case I don't know how is the release supposed to be published on the next week. Probably a second review can be got then. However I don't know who to ask. Maybe Alexander L., since he is back in the team, and he knows the core. >> Sorry, I still couldn't finish with this commit. Made another >> amendment. See diff below, and on top of this commit on the branch. >> >> What I did is I made diag_add_error() assert, when the diag is >> empty. And when we are trying to add not a topmost error. > > Ok, as far as I understand, to ensure that diag_set() is always > called before diag_add(). Exactly. > I added a few comments, check them out: Good.