On 17/10/2012 15:07, Konstantin Kolinko wrote:
> 1. Are <PreResource> and <PostResource> able to inject individual files?
Not yet, but that should be relatively simple to implement.
> 2. I am -1 on removal of VirtualWebappLoader.
>
> The feature of injecting additional classpath entries into
> WebappClassLoader.super.addURL() is an important one and I use it a
> lot to inject external dependent libraries into web applications.
>
> I do not like your approach as a replacement, because classpath
> entries handling (jars handling) in URLClassLoader and in
> WebappClassLoader is based on different requirements. I think that
> URLClassLoader serves better for external libraries
Could you expand on this please. I'm struggling to see the difference
between using the VirtualWebappLoader and mapping JARs into WEB-INF/lib
and directories containing classes onto WEB-INF/classes
> 3. Regarding the API.
> In the tests I see
> [[[
> tomcat.addContext("", System.getProperty("java.io.tmpdir"));
>
> File lib = new File("webapps/examples/WEB-INF/lib");
> - ctx.setAliases("/WEB-INF/lib=" + lib.getCanonicalPath());
> + ctx.setResources(new StandardRoot(ctx));
> + ctx.getResources().createWebResourceSet(
> + WebResourceRoot.ResourceSetType.POST, lib.getAbsolutePath(),
> + "/WEB-INF/lib", "");
> ]]]
>
> That "setResources" call seems to be excessive.
>
> Maybe getResources() should create a StandardRoot instance implicitly,
> or Tomcat.addContext() could create one, or StandardContext could
> create one.
Easy enough. I'll take a look.
> 4. org.apache.catalina.webresources.Cache
> + private ConcurrentMap<String,CachedResource> resourceCache =
> + new ConcurrentHashMap<>();
>
> It think it'd be better with "final" there.
Yep.
> If you resolve #2., then I do not mind the merge.
> I do not see much problems with the new APIs. I think we can go with them.
Great. I'll like to get better clarity on yoru thinking behind #2 before
I merge anything.
> For reference:
> The last merge from trunk to the branch was in r1397302, so I did a
> svn diff between trunk and trunk-resources branch state after that
> commit.
> The command is
> svn diff --old=https://svn.apache.org/repos/asf/tomcat/trunk@1397302
> --new=https://svn.apache.org/repos/asf/tomcat/sandbox/trunk-resources@1397302
>
> I uploaded the diff file here (its size is ~400KB):
> http://people.apache.org/~kkolinko/patches/2012-10-17_tc8_resources-vs-trunk_r1397302.diff
Nice. Thanks.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
