[jsr77][core] AbstractContainer start dependancies

[Greg Wilkins <gr...@mortbay.com>]

Jan has noticed that the current implementation of AbstractContainer.addComponent
creates a start dependancy so that the component must be started before the
container is started.

I think this is the wrong way around, as it is legal in jsr77 for a container to
contain stopped components - else startRecursive would not be needed.

This is causing Jan grief with the webcontainer, as it is insisting that
the webconnectors and webapplications are started before they are
added to the webcontainer - which is not possible.

I have changed this so that the dependancy says that the container must
be started before the component is. It does not appear to break anything else
and it makes Jan's webcontainer start.   So I'll go for a lazy consensus on
this and commit this change tuesday unless anybody objects.

cheers

PS. I still have some major concerns with the naming/structure of these
classes and will shortly propose at least a renaming.

Re: [jsr77][core] AbstractContainer start dependancies

[Dain Sundstrom <da...@coredevelopers.net>]

On Monday, September 15, 2003, at 05:15 PM, Greg Wilkins wrote:

> Dain,
>
> I have not changed anything in CVS yet.  But gianni, jan and I have
> all fixed this locally.
>
> Are you going to change the method signature - or shall I flip the
> arguments for now?

I'll do it in a little bit.  I want to check if anyone else is using it 
the other way.

-dain

Re: [jsr77][core] AbstractContainer start dependancies

[Greg Wilkins <gr...@mortbay.com>]

Dain,

I have not changed anything in CVS yet.  But gianni, jan and I have
all fixed this locally.

Are you going to change the method signature - or shall I flip the
arguments for now?

Dain Sundstrom wrote:
> On Sunday, September 14, 2003, at 07:54 PM, Greg Wilkins wrote:
> 
>> Jan has noticed that the current implementation of 
>> AbstractContainer.addComponent
>> creates a start dependancy so that the component must be started 
>> before the
>> container is started. 
> 
> Really.  That is backwards.  A child can't move to the running state 
> until the parent is in the running state.

exactly.  And a client is allowed to remain stopped even if it's parent
is started.

>> I think this is the wrong way around, as it is legal in jsr77 for a 
>> container to
>> contain stopped components - else startRecursive would not be needed.
>>
>> This is causing Jan grief with the webcontainer, as it is insisting that
>> the webconnectors and webapplications are started before they are
>> added to the webcontainer - which is not possible.
> 
> 
> I'm not sure what you are saying here.  Declaration of a dependency is 
> completely disconnected from the state of either object.

yeh - I didn't say that very well did I.     It is just trying to start
the component BEFORE it starts the container... but the webconnector component
can't start until it has a started webcontainer.

>> I have changed this so that the dependancy says that the container must
>> be started before the component is. It does not appear to break 
>> anything else
>> and it makes Jan's webcontainer start.   So I'll go for a lazy 
>> consensus on
>> this and commit this change tuesday unless anybody objects.
> 
> 
> What did you change?  I can't seem to find it.   Anyway, I think we 
> should change the signature of DependencyService from
> 
> addStartDependency(ObjectName startChild, ObjectName startParent)
> 
> to
> 
> addStartDependency(ObjectName startParent, ObjectName startChild)
> 
> It is just more natural to think parent then child.

what about:

   addStartDependency(ObjectName thisMustBeStarted, ObjectName beforeThisIS)

only half joking :-)

Re: [jsr77][core] AbstractContainer start dependancies

[Dain Sundstrom <da...@coredevelopers.net>]

On Tuesday, September 16, 2003, at 05:30 PM, Greg Wilkins wrote:

> Dain Sundstrom wrote:
>
>> I committed a change to AbstractContainer that fixes this for now, 
>> but this class is going away, so the change is a bit moot.
>
> I don't think it is going away...  it is being renamed 
> ManagedContainer and
> will be based on AbstractManagedObject.  So this code will still be 
> there.

Oh okay.  Same thing to me.

> Are you going to do these changes or shall I?

Go for it.

-dain

/*************************
  * Dain Sundstrom
  * Partner
  * Core Developers Network
  *************************/

Re: [jsr77][core] AbstractContainer start dependancies

[Greg Wilkins <gr...@mortbay.com>]

Dain Sundstrom wrote:

> I committed a change to AbstractContainer that fixes this for now, but 
> this class is going away, so the change is a bit moot.

I don't think it is going away...  it is being renamed ManagedContainer and
will be based on AbstractManagedObject.  So this code will still be there.

Are you going to do these changes or shall I?

cheers

Re: [jsr77][core] AbstractContainer start dependancies

[Dain Sundstrom <da...@coredevelopers.net>]

On Monday, September 15, 2003, at 05:17 PM, Jan Bartel wrote:

>> Anyway, I think we should change the signature of DependencyService 
>> from
>> addStartDependency(ObjectName startChild, ObjectName startParent)
>> to
>> addStartDependency(ObjectName startParent, ObjectName startChild)
>> It is just more natural to think parent then child.
>
> That'll really confuse things :-)  I actually think it is better the 
> way it is,  so you are stating the thing that is dependent before the 
> thing on which it is dependent (in English: THIS depends on THAT).

I was going to disagree on this, because I think the idiomatic usage of 
Parent-Child vastly out weighs the This depends on That usage, but once 
I got into the code I think your way is better.  There are a lot of 
methods in the dependency service that use the This depends on That 
style and on a whole I think it is clear that way.

My feelings on this are not strong, so if someone want to change the 
group opinion, I for one am open.

I committed a change to AbstractContainer that fixes this for now, but 
this class is going away, so the change is a bit moot.

-dain

Re: [jsr77][core] AbstractContainer start dependancies

[Jan Bartel <ja...@mortbay.com>]

Dain,

> On Sunday, September 14, 2003, at 07:54 PM, Greg Wilkins wrote:
> 
>> Jan has noticed that the current implementation of 
>> AbstractContainer.addComponent
>> creates a start dependancy so that the component must be started 
>> before the
>> container is started.
> 
> 
> Really.  That is backwards.  A child can't move to the running state 
> until the parent is in the running state.
Yes, I think the addStartDependency args were typed in in the wrong 
order. I haven't committed the switch around yet.
> 
>> I think this is the wrong way around, as it is legal in jsr77 for a 
>> container to
>> contain stopped components - else startRecursive would not be needed.
>>
>> This is causing Jan grief with the webcontainer, as it is insisting that
>> the webconnectors and webapplications are started before they are
>> added to the webcontainer - which is not possible.
> 
> 
> I'm not sure what you are saying here.  Declaration of a dependency is 
> completely disconnected from the state of either object.
On AbstractContainer, a call to addComponent () results in the addition 
of a start dependency on child to parent (well, it will be when we 
switch the args around). Whenever the Container (courtesy of inheriting 
the handleNotification() method from AbstractStateManageable) receives a 
jmx notification, it re-checks its dependencies to see if it should make 
a lifecycle change (eg start or stop because of it's dependencies).

Because the start dependecy was set up incorrectly, with the parent 
depending on the start state of it's child, when the already started 
parent received jmx notification of the registration of the 
not-yet-started child, the state dependency checking in checkState() 
decided that the parent must stop running because the child wasn't yet 
running.

>> I have changed this so that the dependancy says that the container must
>> be started before the component is. It does not appear to break 
>> anything else
>> and it makes Jan's webcontainer start.   So I'll go for a lazy 
>> consensus on
>> this and commit this change tuesday unless anybody objects.
> 
> 
> What did you change?  I can't seem to find it.  
I don't think the change has been committed yet.

> Anyway, I think we 
> should change the signature of DependencyService from
> 
> addStartDependency(ObjectName startChild, ObjectName startParent)
> 
> to
> 
> addStartDependency(ObjectName startParent, ObjectName startChild)
> 
> It is just more natural to think parent then child.

That'll really confuse things :-)  I actually think it is better the way 
it is,  so you are stating the thing that is dependent before the thing 
on which it is dependent (in English: THIS depends on THAT).

Jan

Re: [jsr77][core] AbstractContainer start dependancies

[Dain Sundstrom <da...@coredevelopers.net>]

On Sunday, September 14, 2003, at 07:54 PM, Greg Wilkins wrote:

> Jan has noticed that the current implementation of 
> AbstractContainer.addComponent
> creates a start dependancy so that the component must be started 
> before the
> container is started.

Really.  That is backwards.  A child can't move to the running state 
until the parent is in the running state.

> I think this is the wrong way around, as it is legal in jsr77 for a 
> container to
> contain stopped components - else startRecursive would not be needed.
>
> This is causing Jan grief with the webcontainer, as it is insisting 
> that
> the webconnectors and webapplications are started before they are
> added to the webcontainer - which is not possible.

I'm not sure what you are saying here.  Declaration of a dependency is 
completely disconnected from the state of either object.

> I have changed this so that the dependancy says that the container must
> be started before the component is. It does not appear to break 
> anything else
> and it makes Jan's webcontainer start.   So I'll go for a lazy 
> consensus on
> this and commit this change tuesday unless anybody objects.

What did you change?  I can't seem to find it.   Anyway, I think we 
should change the signature of DependencyService from

addStartDependency(ObjectName startChild, ObjectName startParent)

to

addStartDependency(ObjectName startParent, ObjectName startChild)

It is just more natural to think parent then child.

-dain