[jetty-dev] Some more DeploymentManager comments

[Greg Wilkins <gregw@xxxxxxxxxxx>]

Joakime,

hope you enjoyed your vacation!

I was going to have an attempt at merging your deploy stuff into trunk, but I've
run out of time.   So I thought I'd give you a bunch more comments and let you
go for it.

These are all mostly minor and in no particular order:


AppLifecycle  - shouldn't that be AppLifeCycle ?


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.


Also not keen on "Phase".  Maybe "Transition" would be a better name?
and surely the values should be DEPLOYING, STARTING, STOPPING, UNDEPLOYING?

Does Step need to be a class? Could it not just be methods on the
Phase/Transition enum?


Thus I think that
   List<Step> getSteps(State from, State to)
really should be
   List<Transition> getTransitions(State from, State to);

Also - can't this method be implemented using a static array of
immutable lists indexed by to/from ?

The AppLifecycle main method should be moved to a test class


The AppProvider methods are just stop/start - so why is this not
a standard Jetty LifeCycle ?


I think the DefaultXxx implementations should be promoted to be in
the o.e.j.deploy package - I hate Impl as a name - as everything is an
impl.

Also, the DeploymentManager should instantiate instances of these
DefaultsXxx - unless there are other implementations provided.
If you have to set them in configuration - then they are not "Default".
If we really can't have them instantiated by default, then SimpleXxx might
be a better name - but I'd really like to be able to have the
DeploymentManager work mostly by default from a simple new DeploymentManager



DeploymentManager.walkLifecycle is not a great name... what about
transitionToState.     Should this be a public method on the manager?


Javadoc should say that an App be registered before walkLifecycle is called?


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 - both would have
more or less the same API's as the old deployers - but inside they would
be implemented with MonitoredDirAppProviders.  They would need to have
a setDeploymentManager method added.    Implementing these would be a good
check to show that we have captured all the existing semantics (or at
least the semantics that we want to keep :)



I'm really not a big fan of classes like AppLifecycleEvent that
are used in methods like:

  onLifecycleBeforePhase(AppLifecycleEvent event)

Can't we just have

  onLifecycleBeforePhase(DeploymentManager manager, App app, Phase phase, State state) ?

actually, if the phase is turned into a Transition and given some methods, then
State can be determined from Transition (as it will always end in the same State).


Finally - I'm still cautious about the BeforeTransition-Transition-AfterTransition
model.    I'm much more familiar with an ExitState-Transition-EnterState model.
I understand why you want the steps - but I can't understand why  before-on-after
handling should be in the same class.  Do you have some concrete examples of
classes that will implement more than one of these methods?

For example, couldn't the WebappVerifierBinding.onLifecycleBeforePhase be
moved to something that tiggers on EnterState for the DEPLOYED state - so
that verification is done on any transition that might result in DEPLOYED state.




Anyway - most of these are minor/stylistic stuff.... but style is important.

Let's have a big push and get this into trunk this week.

cheers