On Jul 6, 2009, at 10:59 PM, David Jencks wrote:
On Jul 6, 2009, at 6:27 PM, David Blevins wrote:
Definitely the code that removes 'java:openejb' and replaces it
with 'openejb' can go in now.
I like the JndiFactory with the createComponentContext() and
getRootContext() methods. I don't like how far through the code
the JndiFactory is spread because of the bind() method. Obviously
all the functionality can be done in the JNDI impl or a wrapped
Context returned from the factory. I don't think the
jndiFactory.bind() communicates anything that can't be done much
better with javadoc on the JndiFactory class and seeing something
with the name 'Factory' passed around like it's a Context is not
too pretty.
A wrapper is possible but I think results in much uglier code for
e.g. xbean. The only non-standard operation is bind-create-
subcontexts so delegating all the other Context methods unchanged
seems a bit like overkill to me. JndiFactory is not an ideal name
but I couldn't think of a better one.
Couple options here. One, we use wrapping already in the embedded API
to offer special Context.close() and Context.bind() functionality to
embedded clients, so there's a ContextWrapper that could be extended
and the resulting code be very small. That wrapper class wouldn't
even need to be xbean-naming specific.
Two, we could just smarten up the xbean-naming code to handle binds
that contain a path where one or more parent names have not been
explicitly created via createSubcontext. At least as an option.
As far as the way the JndiFactory is created, I'd much prefer a
system flag that points to the factory impl class name rather than
have it passed in various constructors. Magic methods and
constructors that aren't used in the main code are so easily broken
and I think make the integration ultimately more fragile. On that
note, having the xbean factory in the OpenEJB code would be
preferable as it would be easier to ensure it doesn't break and
someone could theoretically use the xbean impl even if they weren't
using Geronimo.
Having the xbean jndi factory in openejb would be fine, although I'm
not sure geronmo can use the same wrapper since we are using gbean
features to federate the openejb context into the global context.
Personally I think system flags are usually the work of the devil
and don't see how this case is unusual. Would you set the jndi
factory class name as the system property?
Right, the jndi factory class name would be the value of the new
system property. But now that I look at the code in
OpenEjbSystemGBean I agree that seems like not the best fit. Good
option for people who are bootstrapping via InitialContext or the
future EJBContainer.createEJBContainer(Map<?,?>) API, but not the best
for the integration.
It seems a lot more straightforward to me to allow the embedding
application to supply the context it wants openejb to operate with
using DI which also allows it to construct the context such as jndi
factory as it sees fit rather than requiring a no-arg constructor
which wont really work without really bizarre code in geronimo. So
I hope that even if the normal path involves system properties you
also allow a DI approach.
You are definitely right. I forgot we're already doing this pretty
extensively in the OpenEjbSystemGBean with this technique:
Index: geronimo-openejb/src/main/java/org/apache/geronimo/openejb/
OpenEjbSystemGBean.java
===================================================================
--- geronimo-openejb/src/main/java/org/apache/geronimo/openejb/
OpenEjbSystemGBean.java (revision 785027)
+++ geronimo-openejb/src/main/java/org/apache/geronimo/openejb/
OpenEjbSystemGBean.java (working copy)
@@ -107,7 +107,6 @@
setDefaultProperty("openejb.jndiname.format", "{ejbName}
{interfaceType.annotationName}");
setDefaultProperty("openejb.jndiname.failoncollision",
"false");
- System.setProperty("openejb.naming", "xbean");
if (transactionManager == null) {
throw new NullPointerException("transactionManager is
null");
}
@@ -120,6 +119,10 @@
ApplicationServer applicationServer = new ServerFederation();
SystemInstance.get().setComponent(ApplicationServer.class,
applicationServer);
+ // install xbean naming
+ XBeanJndiFactory jndiFactory = new XBeanJndiFactory();
+ SystemInstance.get().setComponent(JndiFactory.class,
jndiFactory);
+
// install transaction manager
transactionManager = getRawService(kernel,
transactionManager);
TransactionServiceInfo transactionServiceInfo = new
TransactionServiceInfo();
I'll attach my xbean class to the openejb issue.
More than happy to make these changes myself if you prefer; have no
problems putting time in with the suggestions.
Not sure what the strangeness is with the JndiRequestHandler code.
There's no code in there that uses startsWith("openejb/
Deployment"). What kind of problem did you find?
so the deployment tree was the context you get by looking up
"openejb/Deployment" in the root jndi tree.
The request came in with moduleId "openejb/Deployment" and name
"openejb/Deployment/foo". Since the moduleId matched "openejb/
Deployment" the code tried to lookup "openejb/Deployment/foo" in the
deployment tree (see line 123, Context context =
getContext(request);) , which failed in geronimo. I couldn't figure
out where the request contents were coming from and didn't see how
to duplicate the situation with ivm.
That's strange the names are coming in with "openejb/Deployment/" as
the prefix, I don't recall the details on why that might be the case.
Something like this would work:
Index: openejb-ejbd/src/main/java/org/apache/openejb/server/ejbd/
JndiRequestHandler.java
===================================================================
--- openejb-ejbd/src/main/java/org/apache/openejb/server/ejbd/
JndiRequestHandler.java (revision 785410)
+++ openejb-ejbd/src/main/java/org/apache/openejb/server/ejbd/
JndiRequestHandler.java (working copy)
@@ -76,11 +76,13 @@
private Context clientJndiTree;
private Context deploymentsJndiTree;
private final ClusterableRequestHandler clusterableRequestHandler;
+ private Context rootContext;
JndiRequestHandler(EjbDaemon daemon) throws Exception {
ContainerSystem containerSystem =
SystemInstance.get().getComponent(ContainerSystem.class);
ejbJndiTree = (Context)
containerSystem.getJNDIContext().lookup("openejb/remote");
deploymentsJndiTree = (Context)
containerSystem.getJNDIContext().lookup("openejb/Deployment");
+ rootContext = containerSystem.getJNDIContext();
try {
clientJndiTree = (Context)
containerSystem.getJNDIContext().lookup("openejb/client");
} catch (NamingException e) {
@@ -152,7 +154,12 @@
private Context getContext(JNDIRequest req) throws
NamingException {
Context context;
if (req.getModuleId() != null &&
req.getModuleId().equals("openejb/Deployment")){
- context = deploymentsJndiTree;
+ String name = req.getRequestString();
+ if (name.startsWith("openejb/Deployment/")) {
+ context = rootContext;
+ } else {
+ context = deploymentsJndiTree;
+ }
} else if (req.getModuleId() != null && clientJndiTree !=
null) {
context = (Context)
clientJndiTree.lookup(req.getModuleId());
} else {
-David
