EJBTHREE-1454 Encapsulate Container infomation in TO/VO

[jaikiran]

Andrew,

1) Is there any naming convention that is followed for VO in the project?

2) As per the JIRA:

public StatefulSessionProxyFactoryBase(final String name, final String containerName, final String containerGuid,
final JBossSessionBeanMetaData metadata, final ClassLoader classloader, final Advisor advisor)

Where containerName, containerGuid, metadata, CL, and Advisor are all properties of the target container.

I do see that, while registering in JNDI, the jndi context is being passed to the registrar along with the other session container properties. Do you think, we should even encapsulate the jndi context in the VO?

[alrubinger]

"jaikiran" wrote:
1) Is there any naming convention that is followed for VO in the project?

I wonder if we can simplify this further:

The contract between the EJB3 Core Containers and EJB3 Proxy is InvokableContext. Because this whole process of JNDI Binding doesn't cross a network boundary, you can probably skip the whole value object thing and just pass in the Container itself.

So try adding accessors for the properties mentioned to InvokableContext, making sure they line up with the actual method names used by EJB3 Core SessionSpecContainers, and then EJB3 core can do a JndiSessionRegistrar.bindEjb(otherParams,this)...

"jaikiran" wrote:
you think, we should even encapsulate the jndi context in the VO?

Yes, by the same mechanism, use whatever "getContext" or "getInitialContext" is available in EJB3 Core Containers and add this to the InvokableContext contract.

S,
ALR

[jaikiran]

"ALRubinger" wrote:

So try adding accessors for the properties mentioned to InvokableContext, making sure they line up with the actual method names used by EJB3 Core SessionSpecContainers, and then EJB3 core can do a JndiSessionRegistrar.bindEjb(otherParams,this)...

We can introduce these on InvokableContext:

Index: D:/JBoss/EJB3/proxy/src/main/java/org/jboss/ejb3/proxy/container/InvokableContext.java
===================================================================
--- D:/JBoss/EJB3/proxy/src/main/java/org/jboss/ejb3/proxy/container/InvokableContext.java (revision 77145)
+++ D:/JBoss/EJB3/proxy/src/main/java/org/jboss/ejb3/proxy/container/InvokableContext.java (working copy)
@@ -21,9 +21,13 @@
 */
 package org.jboss.ejb3.proxy.container;

+import javax.naming.InitialContext;
+
+import org.jboss.aop.Advisor;
 import org.jboss.aop.joinpoint.Invocation;
 import org.jboss.aop.joinpoint.InvocationResponse;
 import org.jboss.ejb3.common.lang.SerializableMethod;
+import org.jboss.metadata.ejb.jboss.JBossSessionBeanMetaData;

+
+ /**
+ * Returns the initial context
+ * @return
+ */
+ InitialContext getInitialContext();
+
+ /**
+ * Returns the session bean metadata
+ * @return
+ */
+ JBossSessionBeanMetaData getMetaData();
+
+ /**
+ * 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();
 }

Some related questions:

1) The interface InvokableContext sounds generic and probably can be used to carry out any generic invocation. So is it appropriate to introduce getGuid (and similar) method here? What i mean is, do all invocable objects need to have a Guid?

2) The following 2 methods are not available in the EJB3 Core SessionSpecContainers. Introducing these on InvokableContext will require an (straightforward) implementation in the SessionSpecContainers:

String getName(); // Currently the EJB3 core, generates a name based on getObjectName().getCanonicalName()

String getGuid(); // EJB3 Core, currently uses Ejb3Registry.guid(this) to generate this

3) I see that the getAdvisor method is deprecated on the EJBContainer:

 @Deprecated
 public Advisor getAdvisor()
 {
 return beanContainer._getAdvisor();
 }

Any specific reason?

[alrubinger]

"jaikiran" wrote:

+ /**
+ * Returns the initial context
+ * @return
+ */
+ InitialContext getInitialContext();

Let's make that just return Context, otherwise, yup.

"jaikiran" wrote:
1) The interface InvokableContext sounds generic and probably can be used to carry out any generic invocation. So is it appropriate to introduce getGuid (and similar) method here? What i mean is, do all invocable objects need to have a Guid?

Keep in mind that everything here is implicitly pertaining to EJB3 Proxies, so I think all of the methods above are necessary for a contract between the EJB3 Container and the Proxy component (InvokableContext sounding more generic aside). So long as we're making sure that Proxy *needs* all this information, and we're not bleeding EJB3 Core internals outside, we're OK.

"jaikiran" wrote:
2) The following 2 methods are not available in the EJB3 Core SessionSpecContainers. ...

String getName(); // Currently the EJB3 core, generates a name based on getObjectName().getCanonicalName()

getName() is inherited by way of EJBContainer...

"jaikiran" wrote:

String getGuid(); // EJB3 Core, currently uses Ejb3Registry.guid(this) to generate this

True story. Even though the true state is held in Ejb3Registry, I suppose this could be considered an extended property of the container, so go ahead and expose a getGuid() there which delegates back to Ejb3Registry.

"jaikiran" wrote:
3) I see that the getAdvisor method is deprecated on the EJBContainer:

 @Deprecated
 public Advisor getAdvisor()
 {
 return beanContainer._getAdvisor();
 }

Any specific reason?

Yes, because the EJB Containers have been refactored a few too many times and we'd really like to tuck away the underlying Advisor, but really can't just now because too many outside components depend upon its exposure.

S,
ALR

[jaikiran]

Thanks for the comments Andrew. I'll create a patch and attach it to the JIRA (will run the unit tests on all the EJB modules before doing this).

Since the patch would involve changes to the EJB3 core module along with the proxy module, would you recommend that i run the "test-suite" too, to ensure these changes don't break anything?(though the changes are more of refactoring) If yes, is there some document which has the steps to run the test-suite?

[jaikiran]

One more question - The unbindEjb on JndiSessionRegistrarBase currently relies only on the session bean metadata and the context to unbind the ejb.

public void unbindEjb(final Context context, final JBossSessionBeanMetaData smd)

Do you foresee that this method might later require additional information about the container? If yes, do you want me to refactor it to accept the InvokableContext?

[alrubinger]

"jaikiran" wrote:
Since the patch would involve changes to the EJB3 core module along with the proxy module, would you recommend that i run the "test-suite" too, to ensure these changes don't break anything?(though the changes are more of refactoring) If yes, is there some document which has the steps to run the test-suite?

Ordinarily, yes. :)

However, the EJB3 integration testsuite has fallen so far off the mark that you won't be able to extract much information from it. Keep the Unit Tests at 100%, and I'll be able to let you know of problems when I run it against of our internal testsuites.

After my current project is done I can look into restoring the integration tests (and bulking up the Unit Tests to get more coverage).

S,
ALR

[alrubinger]

"jaikiran" wrote:
Do you foresee that this method might later require additional information about the container? If yes, do you want me to refactor it to accept the InvokableContext?

I can't forsee any additional info needed there, but accepting InvokableContext is good for consistency I suppose. :)

S,
ALR

[brian.stansberry]

Having the container name available to proxy-clustered in unbindEjb would be a minor convenience (let's me do some cleanup that would only be necessary if there's a bug). So, count that as a tiny vote for passing the InvokableContext. :-)

[jaikiran]

Ok, I will include this in the patch and update the JIRA soon :)

[alrubinger]

Jaikiran:

Brian's been working on extending the Proxy component for use in a clustered environment, so let's be sure to get his approval on the timing before any commits. The current patch > JIRA > green light > commit approach we've been doing has been working well. :)

S,
ALR

[jaikiran]

"ALRubinger" wrote:
Jaikiran:

Brian's been working on extending the Proxy component for use in a clustered environment, so let's be sure to get his approval on the timing before any commits. The current patch > JIRA > green light > commit approach we've been doing has been working well. :)

S,
ALR

Yes, that's how i plan to do this and other tasks - No commits without review/approval :)

[brian.stansberry]

Thanks. Jaikiran.

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.

[jaikiran]

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.

Sure, i will include these in my changes.

[jaikiran]

"ALRubinger" wrote:

getName() is inherited by way of EJBContainer...

The name in the EJBContainer is being set by the SessionContainer as follows:

super(Ejb3Module.BASE_EJB3_JMX_NAME + ",name=" + ejbName,...);

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. I have provided an overriden implementation in SessionContainer which does this:

 public String getName()
 {
 return this.getObjectName().getCanonicalName();
 }

Question:

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), ...);