* Loaded log from Thu Feb 2 16:18:25 2017 * Now talking on #qpidjava * k-wall (~k-wall@199.253.246.1) has joined #qpidjava * orudyy (~alex@199.253.246.1) has joined #qpidjava * k-wall has quit (Quit: k-wall) * k-wall (~k-wall@199.253.246.1) has joined #qpidjava lorenz__: the more I think about it, the more I think removing the Link class (rather than just clarifying its responsibilities) is a mistake. I think a terminus is associated with a link - I don't think the local terminus *identifies* the link for example it is possible to request that a link moves from consuming from queue A to queue B I am just writing up an alternative design Unfortunately the Spec is very lax on what it requires the broker to do upon such a request. You basically can ignore any change to the local Terminus I *think* that you want configuration change events (including link stealing) to be processed via a Link object… Yes - it's kind of deliberately lax - this was a very long discussion if you want to request that the remote side changes its side of the link, the remote side can either change, or just re-iterate what it wants or, indeed, could do something randomly different I am currently considering a design in which there is a Link object which encapsulates the local and remote Terminus and the LinkEndpoint. Nobody else should have access to them. methods are send-/recv- Flow, Transfer, and Disposition so the LinkEndpoint should be visible to the Session additionally there is a LinkManager instead of a LinkRegistry which lives on the VH and it has two methods attachLink(Session, Attach) -> Link and detachLink(Link, Detach) and indeed, that is really the only thing the Session speaks to personally I feel like it should be LinkManger->get[Receiving|Sending)Link(name, local, remote) -> Link Link.attach(session) -> Future LinkEndpoint.detach() LinkEndpoint.close() interesting actually that's not quite right… it needs to be something like Link.attach(session, local, remote) are those the termini? yeah so LinkManager.get() would always return a non-null link either the existing (unmodified) link, or a newly created one unless local is null I guess actually even then you'd have a link with a null local terminus… which you would then need to immediately close yea, that's the recovery case (i.e. null-lookup) link stealing with those will be so racey ah you mean when the link does not already exist and local is null, then yes creating the link and immediatley closing it would be the right thing link stealing was what got me to the idea of centralising the attach yeah - so it is correct to return a link with a null local terminus, and the responsibility of the session to immediately close it yup so I think having Link.attach(session, local, remote) still allows for "centralisation" because you'll always get the same Link object back maybe just doing synchronized Link.attach() could be enough you'd have to work through all the scenarios… link stealing is just so messy yes, I noticed something like connection A has the link, connection B tries to steal the link, connection A closes the link… if the processing of the close occurs before connection A is informed of B's intention to steal… what happens then? reality collapses on itself :-) you may have to do some sort of reference counting type think on the Link object, so you can't actually remove it from the in-memory cache until it is closed and you know that no-one is referencing it it's crude but syncronizing both Attach and Detach *might* do the trick it won't :-) damn because you need the other session to spontaneously detach before you can reply that you have successfully attached meanwhile all sorts of crap might be happening At worst you'll probably just have to incorporate retry logic into it so that if when your LinkEndpoint future returns you see that the Link has been closed, you simply retry the getLink / attach part In my mind I am seeing a theifQueue where all the sessions wait politely in line until it is their turn to steal the Link. It made me laugh :-) yeah - I hadn't given it a name, but I wondered if a queue would be involved… I guess it would probably need to be but to be honest with futures, synchronized, and a theifQueue I think this is solvable if B tries to steal from A, but C comes along to steal before B ever got the chance to own it exactly yeah - it doesn't seem impossible - just annoying and we'd basically need a way to say "this session/connection cannot ingest any more data from the wire until we've heard back from this future" yes, especially because this is a corner case. but I guess that's usually how it goes. you spend the most time (and code) on stupid corner cases yup but back to the original point of the design. What I like about my idea better is that we have less objects (LinkManager, Link, and Session vs. LinkRegistry, Link, LinkEndpoint, and Session). What I like about your idea better is that at least at face value it seems a bit closer to what the spec describes I am just a bit fearful that the responsibilities of Link and LinkEndpoint become muddled again and we end up with the mess that is the current code. I think if we start off with a clear idea of the responsibilities and document them it will stay better But I guess that should be addressed by having diligent coders that know the spec and the responsibilities that it outlines the issue with the current code is that it is a mix between what was supposed to be "common" code and hackery in the broker to try to use that common code library in ways it was never intended to be used I *hope* you are right. because in general I value your upside (staying close to the spec) over mine the LinkEndpoint stuff was never really written with a broker in mind it was written while the spec was still emerging, really just as a scratchpad where we could do some AMQP experimentation then I had to really quickly hack the broker into using it to get something that looked like it sort of worked for a demo obviously I'm sort of glossing over some details… like how the session/endpoint is made aware of the link being stolen from underneath it... if we weren't glossing over some details it would be called implementation :) also the fact that LinkEndpoint.detach() is not really what happens when the session/connection closes (or when the link encounters an error) rather than the unmapping of the handle but I *think* sticking closer to the spec model of things will end up being clearer from an implementation point of view basically the Link object should not have any state, other than the currently associated LinkEndpoint / the thief queue the Endpoint will have session related state (e.g. the mapping of unsettled deliveries to delivery-ids) the Terminus will have the session independent state (e.g. the unsettled state in terms of delivery tags) just thinking about when a terminus can change. essentially only upon attach and through management (operator changes attributes of a Queue) I don't think the latter actually changes the terminus per se or at least it need not do the terminus is really the consumer (/producer) object rather than the queue itself obviously management actions like deleting a queue/closing a consumer would have an affect on the terminus theoretically we *could* allow management to do things like change a selector on the terminus… that would be kind of weird though :-) whether we would want to communicate something like a queue name change is debatable… there are intentional levels of indirection between the address used by the client to establish the link and the queue name I think updating the Terminus upon management could quickly become messy what was your thinking in requiring the Termini in the get(Receiving|Sending)Link method? shouldn't that be the responsibility of the Link#attach? on the first instance (where the link did not previously exist) I was just using it to pre-populate the link if the link already exists those fields are ignored I'm open to not doing that and simply returning a link where source and target are both null yes, since we need a mechanism to update the Registry upon the Link#attach with changed Termini anyway I am in favour of simplifying the interface sure I added "Alternative Design 2" to the bottom of https://cwiki.apache.org/confluence/display/qpid/Link+Registry+Design I'm not sure that you need remove*Link or even updateLink on the LinkRegistry I think Link should have a fairly intimate relationship with the registry such that when you close the Link, it talks to the registry through a non-public API removeLink? I do not want to fully rely on a timeout. if the link closes I think it should be removed ah ok ^^ see above The only question is about how a management actor might remove / modify a link and again I think that would be some method on the Link itself rather than the registry I'm just not sure (not thought about it) what it would look like Currently I do not envision any of these objects as ConfiguredObjects but I haven't thought about it either but I guess it would be useful to *at least* get some statistics yeah - I don't really see any of this as a configured object but terminus and consumer are pretty much the same thing I think (sorry stepped away for a bit there) so - detach doesn't need to be async either you will first need to process the link stealing notification you have got, or you will be the session associated with the link once you have the linkendpoint association, you can do whatever you like - but once it is stolen then all you can do is acknowledge that (which will trigger the future for the thing that is attaching) and send the link stolen detach on the wire sorry that is a little bit too much implementation without any code for my small monkey brain when I was considering the case of someone detaching while the stealing is ongoing I though having the detach be async would be a good idea. but there might be different ways of approaching/implementing the details of link stealing lorenz__: for safety I think we can make the assertion that only the session whose linkEndpoint is currently attached to a link can perform operations against the link other sessions have to wait until that session has acknowledged the stealing (by, in effect, changing the association between link and endpoint) as such all operations, other than attach can be syncrhonous however there must be some mechanism whereby the session is informed of an asynchronous detach (e.g. link stealing, but also something like queue deletion) processing such asynchronous notifications must be the first thing the session / link does when it is asked to do work in my mind it worked something like this (very vague): the attach calls a asyncSteal which executes on the associated IO-Thread and performs the detach. the future will be true if it sent the detach and false if the link was already detached or closed by something else. I think that way there would be no requirement of checking before everytime before something else happens lorenz__: I don't think you need an async steal… I think the thief never needs to know they are stealing, they just do attach… once the attach succeeds they own the link but the session will be informed when a link is needing to be detached by an aysnc event (whether that be stealing or queue deletion) yes, the asyncSteal was meant to be implementation detail of the attach I agree that when the future of the attach resolves it should either throw an exception or we own the Link So, I think the asyncSteal / asyncDetach / whatever we call it is only responsible for notifying the current owner that they need to give up their ownership. when the current owner gets that, they need to "acknowledge" in some way, which will trigger the change of ownership / queue deletion completion / whatever I think I understand how you want to do stealing. I just wanted to point out that it might not be the only way to do it. I had something different in mind (I don't know whether it would work or which would be better) I'm not sure I understand how your model would work :) yea. I noticed. let me try again :-) you call attach on an already attached Link. that realizes that it needs to steal the link and calls asyncSteal which returns a future. in the asyncSteal we call currentOwningSession.detach(Stolen) which also returns a future. in that detach it switches to the IO thread of that session, sends the detach and then resolves the detachFuture which then resolves the asyncStealFuture. writing this makes me think we can probably cut out one level of indirectio n there that way we would not have to check the stealingRequested flag everytime we want to do something on the link we don't need to do that in my model what? check the flag? yeah so how/when would you detach? in my model you simply need to process the async tasks … the only funky bit is if the link tries to detach when there is an outstanding "detach from someone else" task outstanding processing such asynchronous notifications must be the first thing the session / link does when it is asked to do work So… in my model, someone wants the existing attached session to detach - either because link stealing is going on, or because they want to lose the link for administrative puroposes lorenz__: that's just our standard model of when we process async tasks so that is what I meant by checking the flag. processing async task, checking flag... some say tomato some say tomato (that does not really work when writing ;) but you don't need to do that at every call to anything that is just what session processing already does IIRC the only slightly funky things are dealing with multiple outstanding requests to detach the link (the thief queue), and the fact that in general we are going to have to cope with the situation that we may get stuff on the wire for a link that the server has already half closed the latter we'll need to do whatever solution we choose we have a asynctask list on the connection yeah - so that is probably good enough we just need to make sure that the processing checks that and doesn't ever get stuck only doing IO… which we should make sure of anyway I *think* the way to deal with the thief queue is to say that you only process one async "steal/close" task on the link at a time. So after you process the first one and all the work has been done there, only then do you process the next request on the thief queue… which will now be stealing from the new owner… or having to cope with the fact that the link has already been closed I think the attach(…) and the managementClose(…) [or whatever this second function is] will need to take a function as an argument, the function will actually create the LinkEndpoint (or null, if no LinkEndpoint is to be created), but it will execute on the current owners IO thread - so it shouldn't do any IO work I thought we put the creation of the LinkEndpoint on a Listener on the Future and that Listener should execute on the thread of the new owner I think the association of the link to the new session happens at exactly the point where the old link acknowledges the stealing so I think the LinkEndpoint is created on the IO thread of the "old" connection why? that sets the LinkEndpoint on the future, so the new thread can get it and then action it because you don't want the Link to be sitting around without an associated endpoint… it is owned by someone now I think it's important that the link always has a correct view of whether it is associated to a session or not but why not add a listener to the acknowledgment and let that listner execute on the new owners thread and then fulfill the attachFuture? aha So for me the invariant is that the Link is always associated with at most one session, and that view is "correct"… so you never need to know who you are stealing "from" just that you are wanting to attach the link then processes the attach on the IO thread of the current owner if there is no current owner it can process on the current thread I think that is where in my mind the theifQueue comes into play. if we have a attachInternal which is synchronized we can set a flag attaching in progress and if there is an attach in progress we just add ourselves to the queue. before we return we make sure there is nothing in the theifQueue I can pretty much prove to myself that my way is safe and can be done without races We basically decree that all state changes must be done on the IO thread of the current owner (state changes of the link that is)