Re: [PATCH] CacheManager subscription to IPC

On 08/13/2011 05:59 AM, Amos Jeffries wrote:
> This migrates Cache Manager to using the new IPC subscription API for
> handling message events.
>
> To do this the CacheManager is converted to an AsyncJob which runs from
> the point of first action registration until shutdown. The existing
> event handlers are move into its scope as-is.

> === modified file 'src/CacheManager.h'

> +class CacheManager : public AsyncJob

If CacheManager is a job now, we should not communicate with it using
Instance() and most public methods because it may not be there (or may
not be ready to talk) when we want to talk to it. Jobs should
communicate using async calls.

Please note that I do think CacheManager should be a job [eventually],
but we should change its usage accordingly or, at the very least,
provide temporary controls that bad things do not happen (e.g.,
CacheManager::Instance() returning a stale pointer to the job that has
been destroyed).

> === modified file 'src/cache_manager.cc'

> +void
> +CacheManager::swanSong()
> +{
> + Ipc::ClearListenFor(Ipc::mtCacheMgrRequest);
> + Ipc::ClearListenFor(Ipc::mtCacheMgrResponse);
> +}

> +bool
> +CacheManager::doneAll() const
> +{
> + // Once started, only exit on shutdown.
> + return shutting_down != 0;
> +}

Please call the parent's method at the end of the above methods (even
though the current parent's method may not do anything).

> + Subscription::Pointer sub;
> + RefCount<IpcMsgCall> call;

It is better to avoid creating Pointer-like variables until they are
needed as it saves CPU cycles and simplifies code.

Also, If you can make them const, please do so.

> + debugs(16, 1, "Startup: CacheManager: Accepting manager IPC responses");
> + debugs(16, 1, "Startup: CacheManager: Accepting manager IPC requests");

Use DBG_IMPORTANT if you think these debugging messages warrant level-1
reporting. IMO, they should use level 2.

> + // XXX: Just run the action? we are inside an async manager call already.
> + Mgr::Action::Pointer action = createRequestedAction(request.params);
> + if (Ipc::Coordinator::Instance())
> + AsyncJob::Start(new Mgr::Inquirer(action, request, Ipc::Coordinator::Instance()->strands()));
> + // else throw? coordinator scheduled a call and abandoned us.

This feels very awkward or wrong, but perhaps I am just missing something:

1) Running the action in Coordinator requires a lot of messages. So the
XXX comment cannot apply to the in-Coordinator code. That code does need
a Mgr::Inquirer job!

2) If Ipc::Coordinator::Instance() is nil, I assume the code is running
inside a regular kid. Regular kids should not even know about
Coordinator class and all the stuff that comes with it. The only thing
they should know about Coordinator is how to send it an IPC message. It
is wrong, IMO, for the CacheManager class code to depend on Coordinator
class.

3) The "else throw?" comment looks strange because regular kids do not
instantiate a Coordinator object (I hope!). They should actually perform
the request. I do not see the code that actually runs the action in the
new CacheManager::handleIpcRequest(), which is exactly the place I would
expect that code to be.

Overall, there seems to be some confusion (possibly on my part!) about
what kind of process/class would run the above code. IIRC, it should be
Coordinator process/class, not the general kid process and the general
cache manager class. The cache manager class should have the code to
actually run the action locally, at Coordinator request.

I will stop reviewing here as it will probably be a waste of time
whether I am confused or the code.

Thank you,

Alex.
Received on Wed Aug 31 2011 - 06:02:32 MDT