[GitHub] j4fm opened a new pull request #416: TOMEE-2408 Remove dependency on MP JAR filenames

[GitBox <gi...@apache.org>]

j4fm opened a new pull request #416: TOMEE-2408 Remove dependency on MP JAR filenames
URL: https://github.com/apache/tomee/pull/416
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

Re: [GitHub] j4fm opened a new pull request #416: TOMEE-2408 Remove dependency on MP JAR filenames

[j4fm <ja...@my-managed.net>]

Thanks David, good to work with you too and thanks for the feedback.

This is specifically for MP JARs in tomee/lib folder.  I've run through app
startups with a debugger and it is behaving as intended.  Also, actively
seen it work well against some apps I have here.

I believe the use case is that MP JAR's should be excluded completely from
scanning and their active elements via the tomee.mp.scan setting.  This is
to prevent risk of interfering with existing Plus/Plume based apps/cdi
functionality for people that do not want MP in their Plus/Plume.  So could
be seen as a backwards compat switch.

Secondly, reduces the loading time when tomee.mp.scan = none because
previously everything was being scanned all the time.  MP adds some start up
time for openapi scans, etc.

There is still work to be done to make a setting per-context but that is not
the scope of this PR.

I see JarLocation utility takes a class param.  I could just replace the
whole previous approach and my approach by simply importing the classes and
adding the JarLocations to the scannableUrl.  I was just intending to fix
the filename dependency but am happy to do that too.  It's pretty straight
forward.

Let me know what you think (modularization may make either approach change
anyway).

Thanks again!



--
Sent from: http://tomee-openejb.979440.n4.nabble.com/TomEE-Dev-f982480.html

Re: [GitHub] j4fm opened a new pull request #416: TOMEE-2408 Remove dependency on MP JAR filenames

[David Blevins <da...@gmail.com>]

Hi James!

Thanks for the PR and good goal -- anytime there are jar file names in code, it's a code smell.  I say that having broken that rule myself.

On the PR, my memory of that usage of Class.forName is that looks in the system classpath only and is a repeat source of disappointment.  As the lib/ jars are in a child ClassLoader of the system classloader, I'd expect a ClassNotFoundException every time.

We do have a utility class to try and define jar locations from the class which is used in a handful of places:

 - https://github.com/apache/tomee/blob/master/container/openejb-loader/src/main/java/org/apache/openejb/loader/JarLocation.java#L71

I don't know how well it will stand up to our Java 11 work and the introduction of the module classpath, which as I understand doesn't use file: or jar: URLs.  I guess, however, any motivation to tinker with jars wouldn't fit in that world anyway.  

Do you know why we need the jar locations in the first place?

Side note, I've been noticing a lot of great contributions from you.  Thank you for being here. :)  I'm excited to finally get my turn to work with you :)


-- 
David Blevins
http://twitter.com/dblevins
http://www.tomitribe.com

> On Feb 25, 2019, at 4:01 PM, GitBox <gi...@apache.org> wrote:
> 
> j4fm opened a new pull request #416: TOMEE-2408 Remove dependency on MP JAR filenames
> URL: https://github.com/apache/tomee/pull/416
> 
> 
> 
> 
> ----------------------------------------------------------------
> This is an automated message from the Apache Git Service.
> To respond to the message, please log on GitHub and use the
> URL above to go to the specific comment.
> 
> For queries about this service, please contact Infrastructure at:
> users@infra.apache.org
> 
> 
> With regards,
> Apache Git Services