Hi Amos,
Executive summary: We are making progress as I now understand what
problem you want to solve. We still appear to disagree on the solution,
but it would be easier to find a good one now, and the next step is
clear, IMO. Details below.
On 09/02/2011 12:19 AM, Amos Jeffries wrote:
> Ipc::Coordinator::handleCacheMgrRequest() -> depends libmgr.la
This may be a reasonable dependency, I think: Coordinator needs to be
able to manipulate Cache Manager actions as it aggregates information
collected by them. We can extract that functionality from Coordinator to
another class if really needed. I currently do not see that need.
> libmgr.la -> depends libcomm.la
This is a reasonable dependency because Cache Manager actions need to do
I/O to respond to queries.
> libcomm.la -> depends libipc.la (aka. Ipc::Coordinator::*)
This is probably wrong. Do you know what exactly causes this dependency?
The following yields nothing for me, but you probably know of some
indirect dependences or dependencies in other Comm files:
% fgrep -RI ipc/ src/comm*
> The whole point of this is to break that loop.
I was not aware of that! Please add that motivation to the proposed
commit message / patch description next time around.
I appreciate the problem. Let's find a solution we all can be happy
about. Perhaps we need two IPC libraries: high level unused by Comm
(classes like Coordinator and Strand would go there) and low-level used
by Comm? Perhaps Coordinator and Strand should be moved out to libmgr.
Perhaps there are better alternatives, but let's find what causes the
dependency first.
> With the extra bonus that libipc -> libmgr -> HttpRequest -> Universe
> chain is also erased.
High-level portions of src/ipc/ may need access to CacheManager and
HttpRequest. Low-level portions of src/ipc/ probably do not need those.
> Right now I think Coordinator (and Strand maybe) need to use a _generic_
> ACK message to ACK a certain message ID they sent. This will allow
> removal of the component-specific ACK types from
> Ipc::Coordinator::handle*().
We need custom responses for some of the messages. It is a genuine need
dictated by message semantics. We should not try to remove meaningful
Coordinator responses. The solution is elsewhere.
> Under the subscription model the component specific handle*() are
> simply a table of AsyncCall callback pointers to be scheduled.
There is nothing wrong with that approach as such, of course. However,
the handling code is not going to disappear just because the message
recipient schedules an async call. You still need some class to handle
those messages. Coordinator is currently the right class for the
messages it handles. We can probably redesign/split Coordinator into
smaller independent handlers if the current simple design is in the way
of something.
> Before we go any further I want to make a new patch for that and get you
> to audit that.
I think we disagree on the major design pieces/direction here. It may be
more productive to reach an agreement on that first, starting with
identifying specific loop-causing dependencies.
>>> Allowing us to
>>> NACK it immediately if not handled. Then I can pull all that request
>>> handler Instance() dependency back out of CacheManager.
>>
>> Coordinator accepts all cache manager messages. There should be no
>> reason for sending a NACK to Strand as Coordinator can respond to the
>> client directly if there are any problems with the request it got from
>> the Strand.
>
> What happens if component X (Cache Manager in this instance) is not
> registered to receive that message type?
> My understanding is that it will hang in the sending process until some
> timeout.
If nobody is registered to receive a particular message type, then the
sender will eventually receive a timeout notification. The sender is not
blocked while waiting, of course.
If such timeout is a problem for some specific reason, then we could
introduce a special catch-all handler for this case, but it will be
unrelated to cache manager or linking problems you are trying to solve.
Moreover, there may be better solutions to that specific problem. Let's
_not_ focus on this question for now unless it is somehow tied into the
linking problem.
>> It is possible that I am misreading your intentions, but it feels like
>> you are still trying to fix the original patch which was based on wrong
>> design assumptions. I would recommend starting from scratch and
>> following the steps I outlined at the end of the previous email. In
>> short: subscribe Ipc::Coordinator and Ipc::Strand to receive IPC
>> messages, placing any common code in Ipc::Port. Leave CacheManager as is.
>
> So leaving the core problem of library dependency loops unresolved?
At least we both know what problem you are trying to solve now. Let's
call that progress!
IMHO, the library dependency problem is not caused by the specific
design you are trying to change. It may be caused by incorrect grouping
of IPC classes into libraries. It may be helped by adding new classes
and/or using subscriptions. Etc. I cannot be sure about a specific fix
until I know what exactly causes the comm-ipc dependency loops.
> The goal of this effort is simply to make CacheManager (upper layer)
> subscribe to IPC (lower layer).
As we have learned, this is not the goal. This is a proposed way to
reach the goal of resolving dependency loops.
And I disagree that Ipc::Coordinator is "lower" than CacheManager as far
as layers are concerned. I do agree that some parts of src/ipc/ are lower.
In summary, let's restart by focusing on the core problem: library
dependency and linking loops. If you can identify what makes Comm
dependent on libipc, we can probably come up with a plan on how to break
that dependency.
Thank you,
Alex.
Received on Fri Sep 02 2011 - 21:17:31 MDT
This archive was generated by hypermail 2.2.0 : Sat Sep 03 2011 - 12:00:03 MDT