-
15. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
"jaikiran" wrote:
Relying on the getName implementation of the EJBContainer (which just returns this name), results in failure of a couple of tests in the EJB Core.
This is because EJB Containers are registered with the AOP/Remoting Dispatcher by way of their ObjectName.canonicalName. @see SessionSpecContainer.registerWithAopDispatcher().
I think getName(), in the case of EJB3 Proxy, is vague. And who's to say that the Container is registered for remoting under its name (as you've shown here)?
So rather than overriding getName(), let's add to InvokableContext:String getDispatcherRegistrationName()
...which may be implemented in SessionSpecContainer:return this.getObjectName().getCanonicalName();
...and then be called upon by SessionSpecContainer.registerWithAopDispatcher(). Then you'll have it available to you everywhere within EJB3 Proxy."jaikiran" wrote:
The SessionContainer, is using the deprecated Ejb3Module to create a name for the container. Do you want me to change (and test) the SessionContainer to create the name as follows:super(JavaEEComponentHelper.createObjectName(deployment,ejbName), ...);
A note about SessionContainer:
EJB3 Proxy isn't used until SessionSpecContainer (the difference being that SessionSpecContainer adheres to session beans defined by the spec, and SessionContainer can become the basis for things like @Service).
So where appropriate, I've been slowly breaking things out (and centralizing from StatefulContainer/StatelessContainer) into SessionSpecContainer to try and clean things as we go.
So let's break out the logic in SessionContainer where it constructs a container name into a "protected String constructContainerName(String ejbName)", but leave the implementation as used in the SessionContainer ctor intact.
The end result will be that the container name does not change, but you'll have all information you need from the new getDispatcherRegistrationName().
S,
ALR -
16. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
Andrew,
I have attached a patch to the issue. Please review when you have some free time. -
17. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
* The patch has a series of errors in the Proxy SessionContainerTestCase, references to container.getName(). How were we handling this in InvokableContext - is "getName" valid, or should these references be using "getGuid" or "getDispatcherRegistrationName"?
* We should consider renaming "InvokableContext"; it contains a reference to JBossSessionBeanMetaData, so something denoting that this is related to sessions might be useful. Perhaps we introduce a hierarchy, where "InvokableContext" remains generic, and "InvokableSessionContext" inherits from this and add the "getMetadata" method.
* There are some formatting-only changes in Core SessionSpecContainer. If possible can we exclude these? :)
* Aside from that, the APIs are cleaned, all Unit Tests pass (with some fudging to work around the compiler errors mentioned above), so the only other prereq for a commit is to make sure that the Clustered Integration Tests in the EJB3 TestSuite continue to pass so that we don't give Brian a stroke.
S,
ALR -
18. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
I ran the clustered tests on Friday and saw some errors that weren't there last time I looked a couple weeks ago. I see 4 in ForeignPartitionLocalInterceptorUnitTestCase, 3 in SimpleIsLocalInterceptorUnitTestCase, 1 in NestedBeanUnitTestCase. The last one is expected, the first two are new. All are repeated with buddy replication (BR) enabled and disabled.
I tell you this so you don't freak and think you broke something. I suspect the new ones are some simple deployment problem; will look closer on Monday.
Also, the tests in the org.jboss.ejb3.test.clusteredentity.unit package are not relevant to this work. -
19. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
"ALRubinger" wrote:
* The patch has a series of errors in the Proxy SessionContainerTestCase, references to container.getName(). How were we handling this in InvokableContext - is "getName" valid, or should these references be using "getGuid" or "getDispatcherRegistrationName"?
Andrew,
Are you sure the patch got applied correctly? On my local setup, the SessionContainerTestCase does not have reference to getName. Instead it has been replaced with getDispatcherRegistrationName. All the unit tests pass in my workspace. I rechecked the attached patch and here's an extract:Index: proxy/src/test/java/org/jboss/ejb3/test/proxy/common/container/unit/SessionContainerTestCase.java =================================================================== --- proxy/src/test/java/org/jboss/ejb3/test/proxy/common/container/unit/SessionContainerTestCase.java (revision 78500) +++ proxy/src/test/java/org/jboss/ejb3/test/proxy/common/container/unit/SessionContainerTestCase.java (working copy) @@ -111,16 +111,16 @@ if (sessionContainer != null) { - logger.info("Unbinding: " + sessionContainer.getName()); + logger.info("Unbinding: " + sessionContainer.getDispatcherRegistrationName()); try { - Ejb3RegistrarLocator.locateRegistrar().unbind(sessionContainer.getName()); + Ejb3RegistrarLocator.locateRegistrar().unbind(sessionContainer.getDispatcherRegistrationName()); } catch (NotBoundException nbe) { // we are ok with this exception, which indicates that the test case had // already unbound the ejb related bindings. - logger.debug(sessionContainer.getName() + " was already unbound"); + logger.debug(sessionContainer.getDispatcherRegistrationName() + " was already unbound"); } } -
20. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
"jaikiran" wrote:
Are you sure the patch got applied correctly?
Only so much as:patch -p0 < filename
...would take me. :)
In the code you've posted, now we're saying that the Container will be bound to MC using the AOP Dispatcher registration name. Though they may be equal in value, this doesn't seem very intuitive to me.
I thought we were going to add "getDispatcherRegistrationName" in addition to a "getName" for this reason?
S,
ALR -
21. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
"ALRubinger" wrote:
* There are some formatting-only changes in Core SessionSpecContainer. If possible can we exclude these? :)
Actually, there are quite a lot of formatting changes (which i will revert), but there are API related changes too. Example:Index: core/src/main/java/org/jboss/ejb3/session/SessionSpecContainer.java =================================================================== --- core/src/main/java/org/jboss/ejb3/session/SessionSpecContainer.java (revision 78500) +++ core/src/main/java/org/jboss/ejb3/session/SessionSpecContainer.java (working copy) @Override protected void registerWithAopDispatcher() { - String registrationName = this.getObjectName().getCanonicalName(); + String registrationName = this.getDispatcherRegistrationName();// Bind all appropriate references/factories to Global JNDI for Client access, if a JNDI Registrar is present if (registrar != null) { - String guid = Ejb3Registry.guid(this); - registrar.bindEjb(this.getInitialContext(), this.getMetaData(), this.getClassloader(), this.getObjectName() - .getCanonicalName(), guid, this.getAdvisor()); + registrar.bindEjb(this);@@ -726,7 +709,7 @@ JndiSessionRegistrarBase jndiRegistrar = this.getJndiRegistrar(); if (jndiRegistrar != null) { - jndiRegistrar.unbindEjb(this.getInitialContext(), this.getMetaData()); + jndiRegistrar.unbindEjb(this); -
22. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
"ALRubinger" wrote:
In the code you've posted, now we're saying that the Container will be bound to MC using the AOP Dispatcher registration name. Though they may be equal in value, this doesn't seem very intuitive to me.
I thought we were going to add "getDispatcherRegistrationName" in addition to a "getName" for this reason?
Okay, so we will always use the getName to get hold of the container name and whenever we require the dispatcher name we use the getDispatcherRegistrationName?
I mis-understood your earlier comment:So rather than overriding getName(), let's add to InvokableContext
I thought we were going to replace getName with getDispatcherRegistrationName.
I'll take into account these review comments :)
-
23. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
"jaikiran" wrote:
Actually, there are quite a lot of formatting changes (which i will revert), but there are API related changes too.
Yes, the API changes were all good."jaikiran" wrote:
Okay, so we will always use the getName to get hold of the container name and whenever we require the dispatcher name we use the getDispatcherRegistrationName?
Yup.
S,
ALR -
24. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
Andrew,
I'm done with the changes including the review comments. I am planning to summarize the exact changes and even upload the new patch to the JIRA in my next post.
Brian,
As discussed in this thread,"bstansberry@jboss.com" wrote:
If you refresh your ejb3 checkout, you'll find that there is a new ejb3 module added -- proxy-clustered. The classes in this module are mostly extensions of the ones in proxy that add the behavior necessary for cluster-aware proxies.
Since these classes are extensions, there's a fair amount of overriding of protected methods, including some methods you're probably changing (e.g. JndiSessionRegistrarBase.createJndiReferenceBindingSet(...)). So, you're patch will need to include changes to proxy-clustered as well. AIUI, you're mostly working on passing an InvokableContext param instead of a bunch of details. The changes to proxy-clustered needed to handle that should be simple and pretty obvious.
i have taken care of the necessary changes to the classes in proxy-clustered module. However, i see that the pom of proxy-clustered has a dependency on 0.1.2 version of proxy module:<dependency> <groupId>org.jboss.ejb3</groupId> <artifactId>jboss-ejb3-proxy</artifactId> <version>0.1.2</version> </dependency>
Is there any specific reason for this dependency? For integrating the changes in the proxy module with the proxy-clustered module, i would have to use a higher version of ejb3-proxy. -
25. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
"jaikiran" wrote:
Is there any specific reason for this dependency? For integrating the changes in the proxy module with the proxy-clustered module, i would have to use a higher version of ejb3-proxy.
You may bump up the version in proxy-clustered; for inclusion in AS it's the one defined by ejb3-core that counts.
S,
ALR -
26. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
"jaikiran" wrote:
I am planning to summarize the exact changes and even upload the new patch to the JIRA in my next post.
I have uploded the patch "EJBTHREE-1454 with review comments incorporated.patch" to the JIRA. And here is a summary of what was changed:
1) New methods introduced in InvokableContext:/** * Returns the initial context * @return */ Context getInitialContext(); /** * Returns the classloader associated with this {@link InvokableContext} * @return */ ClassLoader getClassloader(); /** * Returns the name of the container * @return */ String getName(); /** * Returns the global unique id of the container * @return */ String getGuid(); /** * Returns the advisor to use for generated proxies * @return */ Advisor getAdvisor(); /** * Returns the registration name with which the container * has been registered with AOP Dispatcher * * @return */ String getDispatcherRegistrationName();
2) New interface InvokableSessionContext introduced. InvokableSessionContext extends InvokableContext and will contain session specific APIs. Ex:/** * Returns the session bean metadata * @return */ JBossSessionBeanMetaData getMetaData();
3) The SessionContainer (in proxy) and SessionSpecContainer (in core) now implement this InvokableSessionContext. These were earlier implementing InvokableContext.
4) The JndiSessionRegistrar* work with InvokableSessionContext
5) The *Session*ProxyFactory work with InvokableSessionContext whereas the ProxyFactoryBase uses the generic InvokableContext
6) The proxy/proxy-clustered internally use the getDispatcherRegistrationName() API to create the proxy factories. The SessionSpecContainer (in core) too uses this getDispatcherRegistrationName() to register with the AOP dispatcher.
7) Changed the pom in the proxy to build 1.0.7-SNAPSHOT. Changed the pom in core and proxy-clustered to depend on this new 1.0.7-SNAPSHOT from proxy.
Andrew,
I just noticed that there is a StatefulSessionInvokableContext (in proxy) which currently implements the InvokableContext. I guess, this should now implement the new InvokableSessionContext. Do you recommend that i change this too? The patch that i have applied to the JIRA "EJBTHREE-1454 with review comments incorporated.patch" does not include this one change. I will include this change during commit, if approved.
I have done a clean build of all the modules to ensure that all the test cases are passing after these changes. I see 1 testcase running into a error in embedded module, but its not related to this change.
-
27. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
Any additional review comments are welcome :)
-
28. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
"jaikiran" wrote:
I just noticed that there is a StatefulSessionInvokableContext (in proxy) which currently implements the InvokableContext. I guess, this should now implement the new InvokableSessionContext. Do you recommend that i change this too?
Yep.
These look good to the naked eye, and I have all Unit Tests passing. If you can verify the clusteredsession tests in TestSuite are OK, than this is good for commit.
There is one issue that we should probably break off from this work: "getDispatcherRegistrationName()" is being used throughout ProxyFactory code where perhaps "getName()" would be more appropriate.public ProxyFactoryBase(final String proxyFactoryName, InvokableContext container) { // Set properties this.setName(proxyFactoryName); this.setContainerName(container.getDispatcherRegistrationName()); this.setContainerGuid(container.getGuid()); this.setClassLoader(container.getClassloader()); this.setAdvisor(container.getAdvisor()); }
Part of my initial design of the proxy component was based on the assumption that'd we'd be referring to the Container by one name, and this is not necessarily the case (there's a name used to register it in MC/Ejb3Registrar, and another used to register w/ AOP Dispatcher, and these are not guaranteed to be equal). So the solution there would involve adding something to ProxyFactory classes to support each property in the correct place.
Let's open up a JIRA to address that differentiation (and clear up the code so future maintainers don't get confused).
S,
ALR -
29. Re: EJBTHREE-1454 Encapsulate Container infomation in TO/VO
"ALRubinger" wrote:
"jaikiran" wrote:
I just noticed that there is a StatefulSessionInvokableContext (in proxy) which currently implements the InvokableContext. I guess, this should now implement the new InvokableSessionContext. Do you recommend that i change this too?
Yep.
Okay, i included this in my changes."ALRubinger" wrote:
If you can verify the clusteredsession tests in TestSuite are OK, than this is good for commit.
Andrew,
Sorry that this one took really long than what i expected. Mainly because of my lack of understanding on how to get the clustered testcases working in the testsuite. All that i learnt, while trying to get the clusteredsession testcases running, i have noted in this wiki http://wiki.jboss.org/wiki/DevEJB3RunClusteredTestCases. Might help someone else, or even me, the next time i try this :-)
I started off with running the testsuite without including my changes (for EJBTHREE-1454) to get the tests passing and results comparable with the "EJB3 Plugin Against AS5 trunk" http://hudson.jboss.org/hudson/job/EJB3_Plugin_AS_TRUNK/. After a lot of (trial and error) iterations i was able to get them passing and results almost the same as that of hudson (i had a couple of more testcases failing without even including my changes).
I then patched my local JBossAS (Trunk) with the EJBTHREE-1454 changes (followed the "Patch AS" section in this wiki http://wiki.jboss.org/wiki/en/DevEJB3NewPlugin) and ran the clusteredsession testcases again. No new failures/errors were introduced. So the patch looks OK.
Andrew,
Now that the testsuite too looks OK with these changes, do you want me to commit this? Futhermore, the poms that i have changed are:
proxy module:Index: pom.xml =================================================================== --- pom.xml (revision 78909) +++ pom.xml (working copy) @@ -13,7 +13,7 @@ <!-- Artifact Configuration --> <groupId>org.jboss.ejb3</groupId> <artifactId>jboss-ejb3-proxy</artifactId> - <version>0.1.6-SNAPSHOT</version> + <version>0.1.7-SNAPSHOT</version> <name>JBoss EJB 3.0 Proxy</name> <description>JBoss EJB3 Proxy Component</description> <url>http://www.jboss.org/jbossejb3</url>
proxy-clustered module (to depend on the proxy's newer version):Index: pom.xml =================================================================== --- pom.xml (revision 78909) +++ pom.xml (working copy) @@ -90,7 +90,7 @@ <dependency> <groupId>org.jboss.ejb3</groupId> <artifactId>jboss-ejb3-proxy</artifactId> - <version>0.1.2</version> + <version>0.1.7-SNAPSHOT</version> </dependency> <dependency>
core module pom (to depend on the newer versions of proxy and proxy-clustered):Index: pom.xml =================================================================== --- pom.xml (revision 78909) +++ pom.xml (working copy) @@ -365,13 +365,13 @@ <dependency> <groupId>org.jboss.ejb3</groupId> <artifactId>jboss-ejb3-proxy</artifactId> - <version>0.1.6-SNAPSHOT</version> + <version>0.1.7-SNAPSHOT</version> </dependency> <dependency> <groupId>org.jboss.ejb3</groupId> <artifactId>jboss-ejb3-proxy-clustered</artifactId> - <version>0.1.3</version> + <version>0.1.4-SNAPSHOT</version> </dependency> <dependency>
plugin module pom (to integrate these newer version of proxy and proxy-clustered in the JBoss AS):Index: pom.xml =================================================================== --- pom.xml (revision 78909) +++ pom.xml (working copy) @@ -95,14 +95,14 @@ <groupId>org.jboss.ejb3</groupId> <artifactId>jboss-ejb3-proxy</artifactId> <classifier>client</classifier> - <version>0.1.6-SNAPSHOT</version> + <version>0.1.7-SNAPSHOT</version> </dependency> <dependency> <groupId>org.jboss.ejb3</groupId> <artifactId>jboss-ejb3-proxy-clustered</artifactId> <classifier>client</classifier> - <version>0.1.3</version> + <version>0.1.4-SNAPSHOT</version> </dependency> <dependency>