I must admit that when I looked at the code initially I did find it a bit hard to navigate the code. Once I figured out the pattern it was easy.
I understand the reasoning behind Rafi's choice of words for naming the API methods. On the java side some of the names were a bit odd, which looked ok on the C side. But I certainly understand the value of keeping the two implementations as close as possible and naming plays a large part in it. I also agree with Justin that the API method names should be easily understood without having to dig through the code. A well named API is a huge asset in promoting Proton. We want people to dive right into it without having to scratch heads. I did have the advantage of asking questions from Rafi. Without his help I may have struggled a bit. We still haven't made a release yet and now is a good time to have a discussion, rather than a few releases down the line. (I'm not asking to hold onto the current release plans, but asking that we rectify the situation sooner than later). My honest appraisal is that the API is a bit difficult to understand. Perhaps what we need is good documentation and not a name change. Or perhaps we need both. I'm not quite sure yet. I'm planning to write a small tutorial to figure this out myself. I might be able to provide more detailed insight after that. Regards, Rajith On Thu, Sep 13, 2012 at 1:29 PM, Rafael Schloming <r...@alum.mit.edu> wrote: > On Thu, Sep 13, 2012 at 12:07 PM, <jr...@redhat.com> wrote: > >> On Thu, 13 Sep 2012, Rafael Schloming wrote: >> >> On Thu, Sep 13, 2012 at 9:36 AM, Justin Ross <jr...@redhat.com> wrote: >>> >>> >>>> On Thu, 13 Sep 2012, Ted Ross wrote: >>>> >>>> I'm not crazy about the work-processing function names as they seem to >>>> >>>>> disregard the grammar. Should they not all be pn_connection_* >>>>>> functions? >>>>>> >>>>>> >>>>> I agree about this. I would definitely prefer to see pn_connection_* >>>> for >>>> the connection-scoped work interfaces. I guess I thought I was pressing >>>> my >>>> luck, :). >>>> >>> >>> >>> I think there actually already is a consistent rule here, it's just >>> missing >>> from your grammar. Wherever there is a linked list of things, the API uses >>> the form: >>> >>> pn_<collection>_head(pn_<root>**_t) >>> pn_<collection>_next(pn_<**element_t>_t) >>> >>> I think this is better than trying to stick it all on the root or all on >>> the element or splitting it up between the two. For example I think >>> pn_work_head is better than pn_head_delivery as the latter gives you less >>> information. The fact that it is a delivery is already contained in the >>> type signature, and there are multiple lists of deliveries maintained by >>> the engine, so just knowing that it is a list of deliveries isn't >>> sufficient. Even scoping it to the connection is not terribly useful as >>> there may well be multiple lists of deliveries on the connection. The >>> relevant info here is that it is the head of the work queue, a concept >>> that >>> we actually do (or should) explain at length (somewhere). I would argue >>> that the work queue is actually the relevant concept/noun here, it just >>> doesn't have it's own lifecycle since it is a component of the connection. >>> >> >> There's the connection-scoped work queue and then there's what appears to >> be a link-scoped work queue, pn_current/pn_advance. Should it also have >> "work" in its name? >> > > I would say no, I was actually going to comment on your classification > there. There are two linked lists of deliveries: > > 1) The work queue. > > This is scoped to the connection and provides efficient access to > deliveries that are readable, writable, or have had their remote > disposition updated, i.e. deliveries for which some work is expected to > happen. The contents of this list are dynamically updated as conditions > change, e.g. if the remote peer grants or revokes credit or sends more > message data or updates the disposition of a delivery, the contents of the > work queue will change automatically. > > 2) The unsettled deliveries on a link. > > This set of deliveries is scoped to the link, and unlike the work queue it > is not dynamic. It simply contains every delivery for which the link is > tracking state and it is ordered in the exact order that those deliveries > were created, i.e. the send order for outgoing links, and the receive order > for incoming links. Put another way this is the outgoing delivery list for > senders and the incoming delivery list for receivers. > > The pn_current and pn_advance interface doesn't represent a list, but > rather a pointer to a specific delivery within the list of unsettled > deliveries. I would say accessing this and advancing it would fall under > methods on the link, i.e. pn_link_current() and pn_link_advance() in your > proposed scheme. > > Now where the notion of the current delivery does relate to the work queue > is that you can only send and receive data from the current delivery on a > link, so if a delivery is in the work queue because it is readable or > writable, then it is guaranteed to be the current delivery on its > containing link, however just because a delivery is the current delivery on > a link, that doesn't mean it is readable or writable. > > I've tried to put together a picture here illustrating what I'm describing. > Apologies for the ascii art: > > Connection > > work queue -----------+ > | > | > | > | > Link A (outgoing) | +---------+ > \|/ | \|/ > +-----+-----+-----+-----+ > unsettled: | D1 | D2 | D3 | D4 | > +-----+-----+-----+-----+ > current: D3 | > | > +--------------+ > | > Link C (incoming) | > +-----+-----+-----+-----+ | > unsettled: | D1 | D2 | D3 | D4 | | > +-----+-----+-----+-----+ | > current: D2 | > | > +--------+ > | > | > Link D (outgoing) \|/ > +-----+-----+-----+-----+-----+ > unsettled: | D1 | D2 | D3 | D4 | D5 | > +-----+-----+-----+-----+-----+ > current: D4 /|\ | > +-----------+ > > Work queue written out: > (Link B):D1 --> (Link B):D3 --> (Link C):D4 --> (Link C):D2 > > Note that: > > - The work queue provides efficient access to a cross section of > deliveries on more than one link. > > - The work queue is purely a denormalization, you can figure out > everything it tells you by manually iterating over all the unsettled > deliveries on every link and checking whether they are readable, writable, > or updated. > > - Not all links necessarily have deliveries in the work queue, the work > queue only includes "interesting" deliveries (readable, writable, or > updated), and a link may not have any such deliveries. > > --Rafael
