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