[c++] Request for comments: periodic timer implementation for clustering.

[Alan Conway <ac...@redhat.com>]

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

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

Re: [c++] Request for comments: periodic timer implementation for clustering.

[Alan Conway <ac...@redhat.com>]

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

Re: [c++] Request for comments: periodic timer implementation for clustering.

[Andrew Stitcher <as...@redhat.com>]

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. 

Still I've not really looked at code yet, so I may be missing something
really big.

Andrew



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

Re: [c++] Request for comments: periodic timer implementation for clustering.

[Alan Conway <ac...@redhat.com>]

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.
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)

>
> 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?

> 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.

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.

> Hope these random thoughts help.
They do, thanks! Any ideas for a better name?

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

Re: [c++] Request for comments: periodic timer implementation for clustering.

[Andrew Stitcher <as...@redhat.com>]

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.

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.

3. The ClusterSafeTimer implementation should be with the cluster code
not in qpid/sys.

4. I'm unclear whether all Timer events would need to ClusterSafe or not
or just some of them.

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.

Hope these random thoughts help.

Andrew



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