Re: [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area

[Vladislav Shpilevoy <v.shpilevoy@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.