I think I figured out what is a patch. Im going to take this issue as an example. :O)
https://issues.apache.org/jira/browse/OPENEJB-1249 <https://issues.apache.org/jira/browse/OPENEJB-1249>tkx, Thiago. ---------- Forwarded message ---------- From: Thiago Veronezi <[email protected]> Date: Mon, May 31, 2010 at 8:13 AM Subject: Re: @AccessTimeout (was Re: unit test issue) To: [email protected] 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. 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! Thiago. 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 > >
