Re: [Tarantool-patches] [PATCH v2 2/3] fiber: destroy fiber.storage created by iproto

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/01/22 01:25]:
> On 20/01/2020 08:22, Konstantin Osipov wrote:
> > * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/01/20 09:59]:
> >>> Why did you have to add so many invocation points to
> >>> fiber_on_stop() rather than simply adding fiber_on_stop invocation to
> >>> fiber_pool.c?
> >>
> >> We already discussed that in the previous patch version. We decided
> >> to move cleanup to iproto.cc, because it depends on when a request
> >> ends. Fiber pool knows nothing about requests. Iproto.cc is request
> >> processing layer, and this is the right place for request data
> >> destruction.
> > 
> > True, but since you abstracted out the destruction via an opaque
> > trigger, why not move the invocation of the trigger to fiber pool? 
> > fiber pool has most knowledge about fiber life cycle, so it seems
> > natural to invoke the triggers in it - it will tie the *timing* to
> > fiber pool, but not what's going on inside the trigger.
> > 
> > Thoughts?
> 
> And we came to the exactly the same patch I had in the first
> version, with rolled back on_stop -> on_cleanup rename.
> 
> Motivation for calling on_stop in iproto was that only iproto
> is interested in clearing the fiber's state after each request,
> and fiber pool should not decide when a request ends. From what
> I understood. But I am ok with both solutions.
> 
> Anyway, here is the new patch.

lgtm.

-- 
Konstantin Osipov, Moscow, Russia