[jetty-dev] Re: Some more DeploymentManager comments

[Joakim Erdfelt <joakim@xxxxxxxxxxx>]

On Sat, Nov 7, 2009 at 11:36 PM, Greg Wilkins <gregw@xxxxxxxxxxx> wrote:

Joakime,

hope you enjoyed your vacation!

Thanks, it was thoroughly enjoyed. :-)
 

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 ?

Both forms are in common use.
It really depends on if you consider it "Lifecycle" 1 word or "Life Cycle" 2 words.
 

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.

This word should indicate a state where the App is being tracked, but doesn't exist in a form unconnected to anything in Jetty itself.  It has no context, no handler, not started. not stopped.  Can be an old app that has been undeployed, or a new app that hasn't gone through any step in the lifecycle yet.

its almost STATELESS, but that violates lifecycle terminology, and becomes a state that says it has no state, which is confusing.

One tempting path to take is to split UNAVAILABLE into NEW and EXIT, and make the lifecycle open ended.
Reasoning is that when an App is undeployed, it should be removed from the DeploymentManager tracking anyway.
However, There are a few use cases for hanging onto that information. (versioned deployments, staged deployments, rollback deployments)

I'm actually thinking that terms like IDLE or PARKED or UNUSED as concepts is probably the most accurate state name.
This line of thinking could use to be refined tho.

Also not keen on "Phase".  Maybe "Transition" would be a better name?

Traditionally, in a lifecycle you have a Stage/States/Goals/Targets (the Node in the lifecycle graph you want to reach), and the Phases/Step/Actions (the Edges you take from Node to Node).
An Edge can have in 0...n bindings/events associated with it.

If we use Transition for the Edge, then we should use the Phase for the Node, as that is a logical way of refering to it.
"Phase Transition"

I would view Transition in this case refers to the entire process of moving from a Node through the Edge and onwards to another Node.
Transition, in this case, would be too vague and all encompassing.

Naming and language are important points of stablization (more on the thought process below...)
 

and surely the values should be DEPLOYING, STARTING, STOPPING, UNDEPLOYING?

The initial thought process here was to use a VERB for the Edge, and a NOUN for the Node, but the NOUN part proved futile (for obvious reasons).

When looking closer, it was found that the Node and Edge are both Verbs.

Verb can denote a state of being (exist, stand), an action (bring, read), or an occurrence (decompose, glitter)
[from wikipedia]

Knowing that we need to use Verbs, we should then look at the points of interest where the verb is important... the bound listeners.

In an ideal lifecycle, we have the following points / events that people want to hook into.

Edge Leaving Node:        PRE_PHASE
Edge Itself:                    PHASE
Reached Node:               STATE
Error Reaching Node:      ERROR

Example: seeing this in terms of [NODE1] --EDGE--> [NODE2]

[DEPLOYED]  Node we are leaving
    |       PRE_Edge
  START     Edge
    |       Error
    V      
[STARTED]   Node (success)

The linguistics for the verbs initially chosen was passive participle verbs for the Nodes, and standard verbs for the Edge.

Now, your suggestion would change the Edge to be active participle verbs for the Edge.
This seems appropriate.

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 Step object refers to the entire Transition from Node->Edge->Node.
Also, nodes (State) should not be aware of each other (thats inappropriate).
The means from State to State is via an edge (Phase).
The edge (Phase), meanwhile can have knowledge of its two end points.

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.

If you want this to be maintainable, then a proper graph, with BFS, and node to node cache would be the best choice.
Low maintenance, low memory, optimized, and flexible.

 

The AppLifecycle main method should be moved to a test class

Agreed, it was just a temp thing to help generate some unit tests.
 

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.

When I was originally working with Jetty Lifecycle, it became clear that 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.

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.

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.
So far, we have 4 bindings in the o.e.j.deploy base, and 2 more in o.e.j.webapp.deploy (DeployAuditLoggingBinding, and WebappVerifierBinding)

Those last 2 need a better home, I currently have them listed as optional components in the pom.xml, but there is a need for them now, just not a good place for them.
 

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

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

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

Public method makes sense.
This functionality will be exposed for other tooling, such as command line, or JMX, or even a web admin UI (maybe directly, maybe via JMX)

As for the name, how about .obtainState(App, State)
or .achieveState(App, State)
or .attainState(App, State)
or .reachState(App, State)
or .doState(App, State)
or .setState(App, State)

Something that indicates a desire to reach the state, but it is not guaranteed.

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

Good point.
I'll add that to the DeploymentManager class javadoc, and overview javadoc, and AppProvider javadoc.
 

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

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)

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.

This DeploymentManager API is a new API that we want people to utilize and extend from, adding in "internal" or "transitional" classes into the mix will just make things messy to follow, confusing, and imply that we support / maintain those classes as part of the API (which they are not).

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

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.

Again, we are making an API here, it would be a good idea to not uninvent the work of the rest of the Java community just to make things happy for us, the internal API can do what we want, but the API we expose should be familiar, and as stable as possible.  The listener/event object approach allows for the listener API to remain stable, while allowing the event details to grow without forcing a reworking / recompile.
 

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.

Traditionally, the Node->Edge->Node would have been broken down (normalized) into the following structure for the transition from State DEPLOYED to State STARTED

Node Diagram B:
[DEPLOYED]      State
[PRE_START]     Approval Action
[STARTING]      Action
[POST_START]    Success Action
[STARTED]       State

With each [NODE] being a node, not an edge.
And then the binding would be only on nodes.
This is how it was originally developed.  Using a graph of nodes only. Which makes the binding only on the node. and only 1 step/method per binding.

However, in moving to a Listener approach, State becomes special to us, so State becomes the Node, and the rest become pseudo-nodes that are just parts of the edge in the implementation we have.  This now means the Edge has a name as well.  And distinct parts. (oh the mess we are in)

With the verbs above, you can see that there might be more than 1 binding to PRE_START, in which all of those bindings are executed, which can influence the next step from occuring (by throwing an exception).

In the case of various validations, such as the WebAppVerifier, we need to be able to prevent the DEPLOY if the app fails verification.  

Another example, a 3rd party could establish a manual verification process binding to the PRE_DEPLOY, it checks a datastore to see if App Foo is approved for deployment, if it is, it allows it thru, if not it notifies a security person, a sysadmin, and throws an exception preventing the DEPLOY from even occuring.

In the above (Node Diagram B), you'll see that STARTING and STARTED are not tied to each other, this is an interesting disconnect in the nodes, as those 3rd parties wanting to be notified on success/failure have a choice of binding to POST_START or STARTED for success, but no option for failure.   And what would happen if an exception is thrown (accidentally) in POST_START?  does the STARTED state not occur?  The answers to this would likely mean we shouldn't be tieing State to the lifecycle.  But that just complicates the code and API even further

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

Agreed, but keep in mind this is a new API from us, that we want people to use.
So the usual API building themes apply ...

• Documentation
• Stable API (Doesn't mean it cant change, just that the changes are isolated so that the incur minimal effort on the users of the API)
• Familiarity (Common names, themes, styles, techniques, that are in use in many other APIs)
• Examples (Our Default* classes, and Test Cases are a good start here)

 

- Joakim