On 05/11/2011 15:24, Konstantin Kolinko wrote:
> 2011/11/4 Mark Thomas <ma...@apache.org>:
>> On 03/11/2011 17:08, Mark Thomas wrote:
>>> I'm still looking at:
>>> - recovery after fixing the broken file
>>> - not deploying an expanded dir if the context.xml in
>>> conf/Catalina/localhost is broken.
>>
>> OK. Here is a patch for review. There are so many combinations
>> deployment and ways to create a dodgy configuration that I have probably
>> missed a couple of edge cases.
>>
>> The main idea with this patch is that a Context always gets added even
>> if deployment fails. That allows fixes via JMX and prevents
>> WAR/directory deployment after a failed descriptor deployment. If the
>> context.xml can't be read, a new FailedContext object is used. This
>> allows redeploy resources to be tracked but the Context cannot be started.
>>
>> A number of clean-up issues I spotted along the way have already been fixed.
>>
>> http://people.apache.org/~markt/patches/2011-11-03-redeploy-trunk-v2.patch
>>
>
> http://people.apache.org/~kkolinko/patches/2011-11-05_tc8_redeploy-v3.patch
>
> It is just your patch, but
> - reverted renames of "watched resource" -> "reload resource". (*)
> - formatted FailedContext.java and added javadoc at the top of it
> - corrected a typo s/absolue/absolute/
>
> (*) Renaming does not add much, and so it is easier to backport. Let's
> do not rename WatchedResource -> ReloadResource.
Fair enough. Now we aren't exposing redeployResoucres to users there is
no real need for the renaming.
> As of now:
> 1) addReloadResource() is not implemented in FailedContext. So there
> is no way to add dependency on the global conf/context.xml there.
>
> Possible solutions, but need to think about them
> a) store reload resources elsewhere outside of Context
Possible since we no longer allow user configuration of this.
> b) extract some code from StandardContext into a new class (something
> like StandardContextBase?) and share it with FailedContext? I see
> that FailedContext does not inherit from ContainerBase, so maybe there
> is some trouble with that.
I wanted to ensure that FailedContext did the absolute minimum. Any of
those approaches could be made to work. I selected the implement the
interfaces directly approach as it is the simplest to review to ensure
it does what you want it to and no more.
> c) Copy some code into FailedContext?
This would be my preferred choice.
> d) Use StandardContext, but overwrite some methods?
See response to b)
> 2) It is odd that FailedContext has
> @SuppressWarnings("unused")
> public synchronized void addValve(Valve valve)
It needs to support addValve() (I forget what calls this) but it doesn't
need to keep a record of the Valves.
> 3) deployer-howto.xml still mentions "RedeployResource". I'd see where
> it goes and fix later.
>
> PS: I had troubles applying your patch with svn and started a thread
> on users@subversion referring to it as an example. It does not handle
> the way git denotes added files. Not a big deal when there is only one
> added file - it us easy to handle that.
Thanks for the review so far. I'll update the patch.
I'm in two minds whether to apply this patch before or after the 7.0.23
tag. There are clearly issues here that need fixing but the patch is
quite invasive. I'm leaning towards tagging after the patch is applied.
Thoughts?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
