qpid-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alan Conway <acon...@redhat.com>
Subject Re: 'client' APIs again
Date Tue, 12 Aug 2014 09:37:15 GMT
On Tue, 2014-08-12 at 11:25 +0200, Alan Conway wrote:
> On Mon, 2014-08-11 at 16:47 +0100, Gordon Sim wrote:
> > Thanks very much for the comments, Alan, some responses inline...
> > 
> > On 08/11/2014 03:01 PM, Alan Conway wrote:
> > > Naming: I hate things called *_tools, *_utils etc. We should find a more
> > > descriptive name. Not foo or fred or whatchymacallit or dingdong or
> > > doofer either. Ideally these classes might all just end up in the proton
> > > package, or maybe as a subpackage like proton.events or proton.reactor.
> > 
> > I personally think they should (generally) be kept distinct from the 
> > current proton package to make it clear that they are additive and 
> > optional. A subpackage would be ok, I just didn't want to mix this with 
> > dependent edits to the proton tree itself to begin with. It may even be 
> > that not all the additional pieces belong in the same package.
> 
> Fair enough. I like the sub-package but it needs to be a little more
> descriptive than proton.utils. proton.reactor is growing on me a bit.
> proton.events is not so bad - it's all about handling a proton.Event
> after all. 
> 
> > > Runtime is a bit vague as a name as well though I haven't got a better
> > > idea.
> > 
> > Yes, this was unpopular with other I spoke to also. I reverted to my 
> > original name - Container - which isn't ideal either. Any suggestions 
> > from other gratefully considered.
> 
> Its funny how often we run into this problem in this business. "Object",
> "Entity", "Thing", "Class", "Type",
> "IHaveToCallItSomethingButIReallyDontWantToBeSpecific" :)
> 
> Looking at the code, Runtime does nothing but add connect and accept to
> SelectLoop, and SelectLoop is basically an Event pump. I get that we
> need to allow other forms of event pump in future, e.g. EPollLoop or
> whatever.
> 
> So I would first suggest changing the name to
> 
>  EventLoop { connect(); accept(); run(); }
> 
> Since this is really just a "fully functional" general event loop, based
> on some concrete type of event pump implementation (e.g. SelectLoop)
> 
> class EventLoop:
>     def __init__self(self, handlers=None, PumpClass=SelectLoop):
>         self.handlers = handlers or [ default_handlers...]
>         self.loop = PumpClass(Events(ScopedDispatcher(), *handlers)
> 
> Note that as it stands the current Runtime/SelectLoop contract is not
> really suitable for adding new types of pump since RunTime.connect knows
> about Selectables, but that contract is something for integrators or
> advanced users adding new types of pump so lets get the general user
> interface right first.
> 
> I did also consider flipping it and making EventLoop an abstract base
> for SelectLoop and future pumps, but I'm not sure that helps anything. I
> like a separate class that is the end user interface and entirely
> separate classes for implementation details.
> 
> --- python tips (apologies if obvious these are things I only figured
> out recently)
> 
> 1. don't use import *. E.g. use "from proton import Events,...."
> import * makes it easier to write but harder to read since you have to
> consult other files to figure out where things come from.
> 
> 2. NEVER use map/list literals as keyword argument defaults!
> 
> def this_is_bad(handlers=[]): ....
>  
> because the list/map is created at module load time NOT each time the
> function is called!!! Since lists & maps are mutable the default can
> easily be inadvertently changed . Very hard to debug! So do
> 
> def this_is_better(handlers=None):
>   self.handers = handlers or [] # OR
>   if self.handlers is None: ...
> 
> This has the added bonus of being able to differentiate between an
> argument that was not supplied (None) or supplied as an empty list ([])
> if that matters.
> 
> > 
> > >
> > > Get rid of Runtime.DEFAULT. Developers who want a global instance should
> > > make their own and be responsible for its scope:
> > >
> > >      DEFAULT = Runtime()
> > >
> > > We definitely should not provide a global in the library (I have
> > > suffered horribly in the past from this mistake. Seems harmless at first
> > > but creates nightmarish startup & shutdown ordering issues.)
> > 
> > Yes, I was coming to that conclusion myself...
> > 
> > > I'd modify the Runtime constructor a little:
> > >
> > > class Runtime:
> > >      def __init__(self, handlers=None):
> > >          if handlers is None: handlers = [FlowController(10)]
> > >          self.handlers = handlers
> > > That make it possible to be clear about whether you want default
> > > handlers Runtime() or no handlers Runtime([])
> > 
> > Good idea, I'll do that and it's a nicer solution to the defaults than a 
> > global.
> > 
> > > Example ordering: I would put the direct example first. People want to
> > > see something work right away, it is annoying if the first thing you
> > > have to do is set up some extra software. I think the broker example
> > > second would be fine, even if it is a line shorter.
> > 
> > I do see your point and I'll chew this over a bit.
> > 
> > > Waiting for subscriptions with direct connections: one of the things
> > > that bit me hard and often with proton is waiting for things to be
> > > ready. In particular if you create a receiver & sender on separate
> > > connections and send immediately you have a race condition where the
> > > message will sometimes be lost because the sender sent it before the
> > > subscription was actually active. This is quite infuriating to debug.
> > 
> > Yes (unless of course you use some sort of 'broker' that could perhaps 
> > 'queue up' messages it received until there was a subscriber to which 
> > they could be forwarded!)
> 
> I think it's important (and you already have it:) to highlight the
> differences between using a broker and doing direct messaging. This is
> one of the key direct-messaging gotchas to be aware of.
> 
> > 
> > > You dodge the issue by using the same connection for sender & receiver
> > > which is not the case for any but the most trivial example.
> > > I suggest you face this head on:
> > 
> > That's a fair point. I've received other feedback that the examples were 
> > not 'reactive' enough - too much done before the event loop is entered, 
> > not enough done within the loop itself - which fits in with this.
> > 
> > I've already made some changes that address the lack of reactivity by 
> > creating the sender and receiver only when the connection is ready and 
> > sending only when credit has been issued. I could perhaps modify this 
> > further to have the sender created only once the receiver is ready.
> > 
> > The request-response example shows that already of course, since the 
> > receiver's address, generated by the broker, is required before sending 
> > the first message.
> 
> > One of the goals is to emphasise that simple things can be done simply.
> 
> Like Einstein said: Make everything as simple as possible _but no
> simpler_
> 
> > In my first attempt I fell in to the trap of trying to make the first 
> > example so simple that it lost any real illustrative value. However it 
> > is 'hello world' and I don't want to overcomplicate it either, 
> > especially if the issues could be reasonably dealt with in subsequent 
> > examples... just need to find the right balance and all the feedback 
> > helps so thanks again!
> > 
> > > the direct example should show how to
> > > create a receiver on one connection, wait till it is active, then send
> > > on a different connection.
> > 
> > The direct example only actually creates one link (it's a sender from 
> > the "client's" perspective and a receiver from the "server's"). I quite 
> > like that aspect of it so I think your comment is perhaps more 
> > applicable to the non-direct i.e. brokered case.
> 
> Nope, you are missing the point, let me expand.

Actually you are not missing the point - you are right, this applies
when there is an intermediary and you are creating an
address/subscription on the intermediary. It applies particularly to
dispatch which creates addresses implicitly as part of the subscription
process. So this is a narrow issue than I suggested and should be a
separate demo. I'll have a crack at it.


> 
> There is a very common pattern in distributed systems.
> 
> 1. Send notification that you want to receive messages at an address.
> 2. WAIT for the message receiving machinery to be _ready to receive_
> 3. Advertise the address to senders
> 4. Senders send, receivers receive everybody is happy.
> 
> If 1 is asynchronous and you don't do 2 then you have lost messages and
> confusion all around. 
> 
> A broker solves the problem for you by queuing messages so it doesn't
> matter if step 4 happens before 2 completes.
> 
> A hello-world demo over a single connection hides the problem because
> the subscribe and send are serialized over the connection. This is never
> the case in a real-world system. 
> 
> The qpid.messaging API solves this problem by allowing the user to make
> a synchronous subscribe call that does 1 and 2 together.
> 
> Sending a subscribe request in this new API (or in Messenger for that
> matter) *only does step 1*. We need to show people step 2 as well. 
> 
> The new API is perfect as we have access to the subscription-is-ready
> event. I want a demo that shows how to use it and explains why. It can
> be an additional demo if we want to keep the super simple one, although
> I think a 2 connection demo that waits for the subscription would not be
> so complex. Maybe I should stop whining and write it.
> 
> This is not just a demo/test concern. Yes there are patterns where this
> concern does not arise:
> - brokered store and forward
> - systems that are ok with lost messages
> - systems that have a quiet period where they can set things up before
> clients become active.
> 
> However lots of systems need dynamic creation of addresses while the
> system is busy - RPC style systems often fall into this category. Any
> time anything can request a "temporary" address that is not widely
> advertised in advance you can have this problem. 
> 
> This is also not an abstract concern, it is one of the first things I
> hit writing dispatch system tests using Messenger as the client:
> - "why did Ted put: 'while messenger.work(0.1): pass' after every
> messenger.subscribe?"
> - "why do my tests hang and messages vanish all the time?"
> - "ohhh THAT's why"
> 
> Probably working with dispatch, which makes the business of address
> creation and subscription more distributed and slower, makes it more
> likely to hit this issue. But the issue is still there. If you're
> unlucky enough not to hit it in development you can be sure it will bite
> you in production :)
> 
> 
> > 
> > > Reconnect: needs more connection lifecycle events (connected,
> > > heartbeats) but I see you are planning to address this.
> > 
> > Yes. So far I've been making some updates based on feedback received 
> > (examples are all now updated as checked in, though the tutorial text is 
> > still lagging behind a little). However my next step was to look at 
> > timer driven events (e.g. for heartbeats or reconnect backoff) etc, and 
> > some more detailed examples on approaches to reliable messaging.
> > 
> > > Messages and threads: once received an app will want to dispatch
> > > messages based on address and other properties and allocate processing
> > > to threads in various ways. This is (should be) orthogonal to the API
> > > you are designing here but we need to make sure we have adequate thread
> > > safety and don't create limitations in the API that make things
> > > difficult.
> > 
> > I agree that more thought and examples on how to interact with other 
> > threads is important. However at the same time, I want to avoid trying 
> > to make everything threadsafe.
> 
> Agreed.
> 
> > 
> > > E.g. the app may want to process messages from a single
> > > proton connection (i.e. a single event stream) in multiple threads and
> > > send reply messages back to the same connection from arbitrary threads.
> > 
> > The proton connection object is not threadsafe, and I don't think that 
> > should change.
> > 
> 
> Agreed. What I'm getting at is there's a need for more tools (thread
> pools, thread safe queues) to allow people to write multi-threaded
> message servers that aren't necessarily tied to the threading model of
> proton. But I think that should be an add-on not a change to what you
> are doing here, so probably a distraction at this point.
> 
> > Providing utilities to allow message processing on different threads and 
> > have interaction with a given connection coordinated is certainly 
> > something I agree should be explored.
> > 
> > > We should also make it possible for a single thread pool to be used to
> > > handle proton and application events.
> > >
> > > MessagingContext: I found this a bit confusing. I got the impression
> > > that its an abstraction over either a session or a connection, but I am
> > > not too clear what purpose it serves.
> > 
> > Yes, it's (mostly) to allow users to ignore sessions unless they 
> > explicitly want to control them. (It also provided a convenient place to 
> > locate some utility code for creating links in the common cases which is 
> > quite verbose with the engine api as it is).
> > 
> > 
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
> > For additional commands, e-mail: users-help@qpid.apache.org
> > 
> 



---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org


Mime
View raw message