Hi, devs. Thats just to say that I've uploaded a patch for the issues below (here: https://issues.apache.org/jira/browse/OPENEJB-1144) When you have time, please take a look on that for me. tkx, Thiago.
On Mon, May 31, 2010 at 3:38 PM, David Blevins <[email protected]>wrote: > > On May 31, 2010, at 5:13 AM, Thiago Veronezi wrote: > > > Hi, David! > > I've just finished the coding and unit-testing for the ejb-jar stuff: > > http://issues.apache.org/jira/browse/OPENEJB-1144 and > > http://issues.apache.org/jira/browse/OPENEJB-1146. > > Great! > > > I'de like to create a > > patch for that, but Im not sure how to do it. > > Actually, what is a patch? Is it a screenshot of my code with no hidden > svn > > files for you (or the other devs) to verify commit? Where do I publish > it? > > tkx! > > Basically if you type 'svn diff' it will show all the lines of code that > were changed. There's a corresponding program, called 'patch', that will > read the patch file and edit all the code accordingly. > > So you can do something like: > > svn diff > OPENEJB-1144.patch > > And attach that OPENEJB-1144.patch to the OPENEJB-1144 JIRA issue. > > One patch for the two JIRAs is fine. No need to attach the same one twice. > > This page has a tiny bit more info: > > http://wiki.creativecommons.org/HOWTO_Patch > > -David > > > > > > > On Fri, May 28, 2010 at 3:44 PM, David Blevins <[email protected] > >wrote: > > > >> Wow, that was quick. I'm quite shocked actually :) > >> > >> You've definitely got the big picture. > >> > >> On May 28, 2010, at 6:56 AM, Thiago Veronezi wrote: > >> > >>> Hi David, > >>> thanks for the directions. I think thats a good task for me to have a > >> global > >>> view of the system. I think its becoming more clear how to implement > the > >>> fix. Please, verify it for me... > >>> > >>> ************************************ > >>> add new class org.apache.openejb.jee.StatefulTimeoutConfig > >>> > >>> class StatefulTimeoutConfig { > >>> @XmlEnumValue("timeout") TIMEOUT, > >>> @XmlEnumValue("unit") UNIT; > >>> } > >> > >> Note that @StatefulTimeout is a different annotation then @AccessTimeout > -- > >> see spec for details. It has the exact xml schema details too. > >> > >> We will have to support both, so feel free to add that JAXB item while > your > >> in the code. Perhaps a "Timeout" class that could be reused by both. > >> > >> Note also that <access-timeout> is a list as it's possible to specify > the > >> access timeout for each individual method. I'm totally fine if we want > to > >> ignore that in the in the first patch and come back and do it in a > second > >> revision. It's already a lot to chew on and "per method" metadata is > >> tougher to pull through the system. > >> > >> On naming conventions, we typically go with the xml element name with no > >> added suffix. > >> > >>> ************************************ > >>> TransactionType > >>> > >>> Add a new property (say statefulTimeout) to > >>> the org.apache.openejb.jee.SessionBean class (below) > >>> > >>> @XmlElement(name = "stateful-timeout") > >>> protected StatefulTimeoutConfig statefulTimeout; > >>> > >>> ************************************ > >>> Add the new entry (on bold - same org.apache.openejb.jee.SessionBean > >> class) > >>> to the "session-beanType" annotation > >>> > >>> @XmlType(name = "session-beanType", propOrder = { > >>> "descriptions", > >>> "displayNames", > >>> "icon", > >>> "ejbName", > >>> "mappedName", > >>> "home", > >>> "remote", > >>> "localHome", > >>> "local", > >>> "businessLocal", > >>> "businessRemote", > >>> "localBean", > >>> "serviceEndpoint", > >>> "ejbClass", > >>> "sessionType", > >>> "loadOnStartup", > >>> "timeoutMethod", > >>> "initMethod", > >>> "removeMethod", > >>> "transactionType", > >>> "concurrencyType", > >>> "aroundInvoke", > >>> "envEntry", > >>> "ejbRef", > >>> "ejbLocalRef", > >>> "serviceRef", > >>> "resourceRef", > >>> "resourceEnvRef", > >>> "messageDestinationRef", > >>> "persistenceContextRef", > >>> "persistenceUnitRef", > >>> "postConstruct", > >>> "preDestroy", > >>> "postActivate", > >>> "prePassivate", > >>> "securityRoleRef", > >>> "securityIdentity", > >>> "dependsOn", > >>> *"statefulTimeout"* > >>> }) > >> > >> Exactly. Ditto for "acccessTimeout". > >> > >>> ************************************ > >>> Create a new Class... > >>> class AccessTimeoutValue { > >>> long time; > >>> TimeUnit unit; > >>> } > >>> > >>> ************************************ > >>> add new property to org.apache.openejb.core.CoreDeploymentInfo class > >>> > >>> private AccessTimeoutValue accessTimeoutValue; > >>> > >>> AccessTimeoutValue getAccessTimeoutValue() { > >>> return this.accessTimeoutValue; > >>> } > >>> > >>> void setAccessTimeoutValue(AccessTimeoutValue accessTimeoutValue) { > >>> this.accessTimeoutValue = accessTimeoutValue; > >>> } > >> > >> For the CoreDeploymentInfo class we can use an existing class called > >> org.apache.openejb.util.Duration. It does some other fancy stuff, but > >> basically it's a long/TimeUnit tuple. > >> > >> As noted above, feel free to ignore the fact that AccessTimeout can be > done > >> on a per method basis. Eventually we'll need some map here, but it's > fine > >> to ignore because I have some fancy ideas for what we should use as the > >> default when they haven't specified the AccessTimeout for a specific > method. > >> > >> Maybe better to get something basic done and checked in. I suspect the > >> code will be changing kind of fast with all the other EJB 3.1 work we > have > >> going on and holding big changes can be hard. In fact, feel free to > submit > >> patches for individual little parts of this while chunk of work. I > tried to > >> break it up into sort of logical "bites" in our road map: > >> > >> http://openejb.apache.org/ejb-31-roadmap.html > >> > >> Feel free to attach patches to those individual jiras. > >> > >>> ************************************ > >>> Class org.apache.openejb.assembler.classic.EnterpriseBeanInfo > >>> > >>> add new property > >>> public AccessTimeoutValue accessTimeout; > >>> > >>> ************************************ > >>> > >>> Class org.apache.openejb.config.EjbJarInfoBuilder (line 564) > >>> > >>> bean.accessTimeout = new AccessTimeoutValue( > >>> Long.valueof(s.geStatefulTimeout().TIMEOUT), > >>> Long.valueof(s.geStatefulTimeout().UNIT)); > >> > >> To match the Timeout class of the jaxb tree, we can make a TimeoutInfo > >> class. See OpenEjbConfigurationValidationTest for the rules we have in > >> place. > >> > >>> ************************************ > >>> Class org.apache.openejb.core.stateful.StatefulContainer. > >> [...] > >> > >>> Use... > >>> synchronized (instance) { > >>> *final AccessTimeoutValue accessTimeout = > >>> instance.deploymentInfo.getAccessTimeoutValue();* > >>> > >>> Lock currLock = instance.getLock(); > >>> final boolean lockAcquired; > >>> if(accessTimeout == null) { > >>> // returns immediately true if the lock is available > >>> lockAcquired = currLock.tryLock(); > >>> } else { > >>> // AccessTimeout annotation found. > >>> // Trying to get the lock within the specified period. > >>> try { > >>> lockAcquired = currLock.tryLock(accessTimeout.value(), > >>> accessTimeout.unit()); > >>> } catch (InterruptedException e) { > >>> throw new ApplicationException("Unable to get lock.", e); > >>> } > >>> } > >>> // Did we acquire the lock to the current execution? > >>> if (!lockAcquired) { > >>> throw new ApplicationException( > >>> new ConcurrentAccessTimeoutException("Unable to get > lock.")); > >>> } > >> > >> This is the part that is a little tricky. Basically all of that > >> synchronized block has to get rewritten. The "gotcha" is that if > someone is > >> waiting (tryAquire(1, MINUTE)) inside a synchronized block, then all > other > >> threads trying to access that instance will also have to wait just to > enter > >> the synchronized block and get their chance to call "tryAquire", which > if > >> course involves more waiting. > >> > >> I'm not entirely sure we need the synchronized block anymore. At best we > >> need it after the lock aquire. That of course makes the "tryLock()" > call > >> already happening a bit strange. > >> > >> Will probably take some experimentation/testing to get it nailed down. > >> > >> -David > >> > >> > >
