qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alan Conway <acon...@redhat.com>
Subject Re: [c++] Request for comments: periodic timer implementation for clustering.
Date Wed, 27 Jan 2010 22:18:53 GMT
On 01/27/2010 04:31 PM, Andrew Stitcher wrote:
> On Wed, 2010-01-27 at 15:54 -0500, Alan Conway wrote:
>> On 01/27/2010 10:21 AM, Andrew Stitcher wrote:
>>> On Tue, 2010-01-26 at 17:27 -0500, Alan Conway wrote:
>>>> A work-in-progress fix to the management timer updates problem in a cluster.
>>>> Would appreciate any comments in particular on ways to get around the ordering
>>>> problem which not obvious to me:
>>>>
>>>> https://issues.apache.org/jira/browse/QPID-2364
>>>
>>> I haven't had a thorough look yet but some things come to mind:
>>>
>>> 1. If you've made Timer an interface with a small number of
>>> implementations then I think this is the correct way to go. It's not
>>> clear to me this is actually what you did.
>>
>> I've added a second type of timer with 3 implementations. The existing Timer
>> remains for 3 reasons:
>> 1. It supports absolute as well as periodic tasks and for this I only want to
>> support periodic tasks
>> 2. Tasks for the new timer must be named so they can be co-ordinated across the
>> cluster.
>
> 1. You can still use the same interface even if you don't use all of it.
> I don't think any absolute timers exist though, just non periodic ones
> (ie one shot). And I don't see why you'd want to exclude those even if
> you don't currently use them.
>
> 2. I think all the current uses of the timer should be named too, for
> diagnostic reasons
>
> So in short I don't see any need to introduce a new timer interface.
>
>> 3. There are several existing uses of Timer which should _not_ be multicast as
>> they deal with things without cluster-wide relevance (e.g. heartbeats) or that
>> already have cluster-safe solutions (e.g. message TTL)
>
> I think that would be dealt with by having 2 timers, their interface
> should still be the same as far as clients of the timer can tell. In any
> event you've got to be able figure out that you need a cluster
> coordinated timer event, once you've worked that out the fact the it
> doesn't do one shot events isn't important.
>
>>
>>>
>>> 2. PeriodicTimer is a crummy name. Call the Base interface Timer as it
>>> is already or TimerBase. Then call the implementations StandaloneTimer
>>> and ClusterSafeTimer or some names like that.
>>
>> It is a crummy name. The name needs to be distinct from Timer, and should convey
>> "timer that you should use for periodic tasks that need to be co-ordinated in a
>> cluster". Any suggestions?
>
> As above I disagree with this logic; I still think you should rename the
> current Timer to StandaloneTimer or near equiv and use ClusterTimer or
> near equiv. and actually do everything in practice with TimerBase or
> Timer interfaces (depending whether you want to avoid renaming all
> current uses of Timer or not).
>
>>
>>> 3. The ClusterSafeTimer implementation should be with the cluster code
>>> not in qpid/sys.
>> It is.
>>
>>> 4. I'm unclear whether all Timer events would need to ClusterSafe or not
>>> or just some of them.
>> Just some see above. Currently only management periodic processing.
>>
>>> 5. To get round the ordering issue perhaps hang on the multiphase plugin
>>> load. Ie change Management init until all plugins loaded. Management
>>> isn't a plugin itself is it. Perhaps I missed an important issue that
>>> stops you doing this.
>>>
>>> Maybe you need to register the ClusterSafeTimer with the Broker object
>>> very early in the plugin init.
>>>
>> The trick is that management registers its tasks *before* plugin init.
>
> Any reason why this is actually necessary? why not just make the Mgmnt
> code register timers after plugin init?
>
>>
>> I solved this with a 3rd timer implementation. It stores up tasks that are added
>> during init and delegates them to the real impl. The real impl is supplied by
>> the cluster plugin or broker supplies the standalone impl at the end of init if
>> there's no cluster plugin.
>
> This just needs to go! It's a potential minefield of misunderstanding
> etc.
>

I think I agree with all of this. I'm going to commit my current code as it 
works and fixes a serious bug, but I'll follow up with a rename and refactor 
along the lines you suggest.

---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project:      http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org


Mime
View raw message