[jetty-dev] Re: Some more DeploymentManager comments

[Greg Wilkins <gregw@xxxxxxxxxxx>]

Joakim,

I'm replying to the simple points below, but will ponder some of
the longer responses a bit longer....


Joakim Erdfelt wrote:
>     AppLifecycle  - shouldn't that be AppLifeCycle ?
> Both forms are in common use.
> It really depends on if you consider it "Lifecycle" 1 word or "Life
> Cycle" 2 words.

Jetty consistently uses LifeCycle, so can we go with that.

>     AppLifecycle.State.UNAVAILABLE - I still don't like this name as it
>     clashes with a STARTED web app that is in unavailable state and serving
>     503's.      What about UNDEPLOYED instead?   That matches with the
>     UNDEPLOY phase.
> 
> 
> UNAVAILABLE hasn't really sat well with me either, as its not accurate
> enough.
> but UNDEPLOYED isn't accurate either, as that would indicate that the
> app has gone through the lifecycle and has been undeployed.

I think undeployed equally applies to something that has not yet been deployed.
While I conceed that it's not perfect - it is the best name suggested
so far.  Let's go with that until something that is universally agreed
is better is suggested.




> As for making them immutable lists, that level of optimization for
> deployment is rather aggressive, and high on maintenance, for how
> infrequent deployment actually occurs.

OK


>     The AppProvider methods are just stop/start - so why is this not
>     a standard Jetty LifeCycle ?
> 
> 
> I started out down this path, but it quickly proved inappropriate.
> 
> The AppProviders are maintained by the DeploymentManager.
> The DeploymentManager in turn is a Jetty Lifecycle, and it delegates the
> start/stop to the AppProviders, along with providing DeploymentManager
> references to that instance of the AppProvider.
> 
> The AppProvider to DeploymentManager relationship needs to be
> maintained, as we have a use case for multiple AppProvider references
> (eg: monitor different dirs) and even multiple DeploymentManager
> instances (eg: dev / prod and even standard / virtual host separation,
> based on multiple ContextHandlerCollection objects needing to be managed.)
> 
> There is also a usecase for AppProviders to be individually stopped.

Jetty is constructed of many many LifeCycle instances with simple
and complex relationships and for components that can be start/stopped
together and/or individually.

The key pattern that the start mechanism captures with AbstractLifeCycle
is that start() and stop() are final and the work is done in doStart()
and doStop().  This avoid a whole bunch of issues with inheritance, encapsulation
and aggregation of other lifecycles.   It is a pattern that was derived
from working with JSR77, jboss and geronimo lifecycles and is simple yet
flexible.

I think any major component of jetty that needs to be start/stopped
should use LifeCycle if not AbstractLifeCycle.  Note only does this
give consistency it allows the standard JMX MBean for LifeCycle to
be transparently applied so the components can be monitored and
controlled from jconsole or similar.


> When I was originally working with Jetty Lifecycle, it became clear tha
> the DeploymentManager would wind up implementing many of the Jetty
> Lifecycle Management pieces itself just to accomplish start/stop.  The
> rest of Jetty Lifecycle
> starting/stopping/stopped/started/failed/listeners was just too much
> baggage to drag along into a provider.

Then just use AbstractLifecycle and implement doStart and doStop.

The benefits of consistency and uniformity outweigh and slight
over engineering.   Besides I would believe that starting would
not be atomic and would have potential to fail, so I really think
LifeCycle should be used.


> This was for code organization purposes.
> If o.e.j.deploy.impl is frowned upon, then we should look for some other
> package name that indicates that these are the standard/optional
> bindings, and not part of the DeploymentManager core API.
> Dragging them into o.e.j.deploy is just muddying the waters, and implies
> that they are core and required.

I can see that argument - but I guess the rest of Jetty takes this
approach - so a package can provide both an API and a default impl.

If a name can be thought of that applies and is not impl, then I
think it's fine to have the sub-package.  But if there is not
good name -then I think that implies there is not good case for
the package.


>     It would also be great to get some transition classes to make it easy to
>     move from ContextDeployer and WebAppDeployer.    Would it be possible to
>     get ManagedContextDeployer and ManagedWebAppDeployer - ...
> 
> Not all of the functionality has been transitioned over. (re: ugly allowDuplicates on WebAppDeployer)
> And there's also broken functionality in the old deployers fixed in MonitoredDirAppProviders (re: recursive, dir with .war or .jar at end of name, root name reference, and filename filters)

Leave the ugly methods - but have them either as noops and/or warnings.

> Otherwise, not a bad idea, but it would make the code organization messy.
> Can these exist in jetty-deploy-transitional project? just to keep them out of the o.e.j.deploy package space of the deployment manager.

I don't think they need another project - they should be part of jetty-deploy,
but they can be in a subpackage like o.e.j.deploy.legacy.ContextDeployer
and marked as deprecated.     They would only last a short while,
until others have direct experience with the new deployer manager.




> This is indeed planned. to have them be automatically bound, but base
> that on a configurable for the DeploymentManager.
> This is not implemented (yet).
>  
> As for Simple* vs Default*, how about Standard* ? or Jetty* ?

If it is planned to make them Default - then Default* works for me!


>     I'm really not a big fan of classes like AppLifecycleEvent that
>     are used in methods like:
> 
>      onLifecycleBeforePhase(AppLifecycleEvent event)

> I went down this path originally, but the method started out with over 12 arguments (something I abhor).
> So it was made into a formal event object to match the core Java listener/event mechanism.

OK.





cheers


PS.  The main matter I've not addressed here are the comments about
the state machine.    I'm understanding all the words your using, but
not exactly how you're using them together :(  So we're having
a disconnect about how we see this state machine working (actually
I think we agree on how it works and the things it does - just not
the labels we apply to them).   But I don't think that is an issue
that need delay the code going into trunk, specially if the above
niggles are fixed.    We can then have a call to reconcile our
world view of state machines.