openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kevin Sutter (JIRA)" <j...@apache.org>
Subject [jira] Commented: (OPENJPA-952) Utilize Sun JDK's Attach API to dynamically load the OpenJPA enhancer agent
Date Fri, 13 Mar 2009 14:41:50 GMT

    [ https://issues.apache.org/jira/browse/OPENJPA-952?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12681733#action_12681733
] 

Kevin Sutter commented on OPENJPA-952:
--------------------------------------

Rick,
Thanks for the patch.  I like the idea behind this patch and the JIRA itself, but I have a
few questions and comments...

o  Testing.  We need a means of testing this approach on a regular basis.  One of the problems
with the old subclassing support is that it wasn't exercised.  We didn't know about some of
the problems until users ran into them.  Somehow, we need the ability to run the junit bucket
or a proper subset of the bucket using this dynamic agent support.  Could a new testing profile
be created to make this easy to execute?  Maybe it's not run with every test build, but we
easily have the capability to run the bucket?

o  Documentation.  As you have found out, we're short on documentation in this enhancement
area.  I don't see any doc updates in the patch you provided.  If we put this support in place,
then we need some documentation to explain this new capability.  It's there automatically
with Sun Java6, but is there any way to turn it off?  Should there be?  What about the other
existing options like oopenjpa.RuntimeEnhancedClasses?  Any conflicts with these option settings?

o  Javadoc.  I noticed a couple of places where javadoc probably needed to be updated, like
with the PCEnhancerAgent.  The javadoc in this class talks about the -javaagent processing.
 This would be a good area to update with the information on the dynamic agent plugin.  In
general, don't forget about the javadoc.  It's very important.

o  Java2 Security.  Due to the jar file manipulation and classloading that you are attempting
to do, these type of changes need to be executed with Java2 Security turned on.  Albert has
provided an easy way to run the bucket or an individual test with Java2 Security turned on.
 It's something like this (but don't quote me):

    mvn myTest -Penable-security

o  The flags and methods in PCEnhancerAgent don't synch up for me.  For example, you introduced
a boolean called enhancementComplete, but the accessor method is hasRun().  Since enhancementComplete
seems to be set to true when preMain is first called, enhancement isn't really complete, is
it?  Something along the lines of resident, invoked, initialized, or something like that seems
more appropriate.  And, then having an accessor method that matches the boolean would be better.

o  Warning mesasge id of entities-loaded-before-em.  I thought we could key off the emf creation
instead of the em.  Would it be better to re-name and re-word this message to just indicate
that entities were accessed before the dynamic agent was configured.  And, if the EMF creation
is the proper step-off point, then it's okay to use that for an explanation and resolution.

o  Trace messages.  It looks like you are trying to introduce some type of "eye catcher" for
trace messages (ie. the ":157" in the trace message for ManagedClassSubclasser).  This is
not consistent with the rest of the OpenJPA trace messages.  So, unless you are proposing
to introduce a change to all of the Trace message processing, then I'd stick with the current
conventions.  :-)

o  I see that you modified the existing logic of just warning a message when RuntimeUnenhancedClasses
is set to WARN.  If it's something other than WARN, then you now throw an Exception.  What
about a value of Supported?  And, is this the right place to throw this exception?  At minimum,
we need more comments as to why we're changing the existing processing.  I'm not clear on
why an exception is proper processing at this point.

o  The indentation and general formatting doesn't seem to be consistent.  In one spot, your
indentation is the expected 4 spaces, while in other spaces it's all the way out to 8 spaces.
 Are you using space substitution for tabs?  And, have you checked for the 80 column limit?

o  Nit.  Check the spelling in your comments and javadoc updates.  Just read through them
and I think you'll find a few.

o  Some of the Trace statements in PersistenceProviderImpl need more "beef".  They should
be self-explanatory by reading a Trace log file without having to reference the code.  For
example, 

    _log.trace(_name + ":468",t);

    ... wouldn't tell me enough in the log file as to why this was logged.  Some additional
text explaining what this dumped exception means to me would help with deciphering a log without
relying on the code.

o  Can we be more helpful with the necessary runtime environment?  For example, if we detect
they are running with the Sun JDK 6 JRE instead of the SDK (ie. no tools directory), could
we output a message that would tell them about this nice feature if they would only run with
the SDK...  :-)

o  I like the getAgentJar processing for when running within Eclipse.  Should make testing
even easier.  Thanks.  But...  (always a but)...  Shouldn't there be a way to turn this feature
off?  Just in case we're debugging a particular situation, I think we need a means of turning
off this dynamic support.  Don't you think?

o  All external messages (non Trace) needs to go into our message files.  Developers want
the ability to read translated messages, much like end users.

That's it.  Thanks again for driving this JIRA.  I think we really need this function and
I appreciate your efforts.  When you have an updated patch, I'd be happy to review it again.

Kevin

> Utilize Sun JDK's Attach API to dynamically load the OpenJPA enhancer agent
> ---------------------------------------------------------------------------
>
>                 Key: OPENJPA-952
>                 URL: https://issues.apache.org/jira/browse/OPENJPA-952
>             Project: OpenJPA
>          Issue Type: Improvement
>          Components: kernel
>    Affects Versions: 2.0.0
>         Environment: Sun 1.6 JDK. 
> Note: The Attach API is ONLY a part of the JDK, not the SDK.
>            Reporter: Rick Curtis
>         Attachments: OPENJPA-952.patch
>
>
> When running in a JSE environment, OpenJPA could use the Attach API to dynamically load
the enhancer agent at runtime. Dynamically loading the enhancer means that an OpenJPA developer
doesn't need to configure a -javaagent. Doing this would dramatically improve the out of box
performance, and also improve the ease of use. 
> This improvement has the following caveats:
> 1.) This API is ONLY a part of the 1.6 JDK.
> 2.) This API is supported by only the Sun JDK.
> 3.) If the agent is loaded from the earliest OpenJPA code, the agent will be laoded when
creating an EntityManager in the EntityManagerFactoryImpl. If an Entity class is loaded by
the JVM before the enhancer agent is loaded, that class' byte code will not be enhanced. 
> Attach API - http://java.sun.com/javase/6/docs/technotes/guides/attach/index.html

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message