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.