Finally got some free time to respond again :)
On Jan 24, 2005, at 7:28 PM, Mark wrote:
Thanks for the review. I will go through your comments one-by-one over the next few days.
A few things to keep in mind:
1. The rmi/iiop code is fairly solid (with my fingers crossed). I hope to get in a bunch of test cases that following the Geronimo design.
Excellent!
2. The stub/skeleton generation is farily *dirty* it. You can convience it to work, code generation style/speed were not my primary objective. The rmi-iiop was. So I fully anticipate this to change in the near future.
Yep. The Geronimo deployment code is similar; not fast or that beautiful, but works.
3. The original code was stand alone, hence there are a few other interop.* packages. Once I learn more about Geronimo, I anticipate a much neater/tighter integration. Of course, I think I will have lots of help from Alan. :-)
I just wanted to point out some of the stuff I noticed. I figure the neat/tight integration will take long time (months) as all of us get accustomed to new integrated code base.
4. I don't mind making the code or style changes to conform tothe Apache project. The current style is a required habit for other projects that I work on. :-)
I've added a few quick answers below see MD>>
( I only made it 1/2 way through this email... )
Expect that I will be submitting lots more changes to Alan over the comming days.
Great... the rest of my comments are inline (I sniped out the stuff where I don't have any other comments... darn thing is just too long :)
Dain Sundstrom wrote:
Added: geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ CosNaming/iiop_stubs/NamingContext_Stub.1.txt
Url: http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/ java/ org/apache/geronimo/interop/CosNaming/iiop_stubs/ NamingContext_Stub.1.txt?view=auto&rev=126264
Added: geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ CosNaming/iiop_stubs/NamingContext_Stub.java
Url: http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/ java/ org/apache/geronimo/interop/CosNaming/iiop_stubs/ NamingContext_Stub.java?view=auto&rev=126264
Are these generated stubs based on the IDL above? Normally, we don't check in generated code, but I bet these almost never change.
MD>> The name service stubs/skels are generated. Right now, due to a couple of issues, I have hand modified them and simply checked them in. This will need to change. All other generated files go under interop/target/src
I see. You many want to put a big warning at the top saying "don't bother messing with this is generated and going away".
Added: geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ InteropGBean.java
Url: http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/ java/ org/apache/geronimo/interop/InteropGBean.java?view=auto&rev=126264
===================================================================== == =======
--- (empty file)
+++ geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ InteropGBean.java Sun Jan 23 23:33:10 2005
+/** + * A GBean that provides an example interop + * + * @version $Rev: $ $Date: $ + */ +public class InteropGBean implements GBeanLifecycle {
Is this test code? If it is an example we want to ship with the server, maybe we should put it in an example package or rename it ExampleInteropGBean.
MD>> This GBean was the interop/corba container loader. maybe a better name is in order.
I think you lost me. What is it? The reason I thought is was test code was because it had an echo method that looks like test code I normally write.
I haven't had a chance to flesh out the details for the Geronimo integration. I'd like to see the interop container be loaded by the InteropGBean. The InteropGBean could be a seperate plan or as part of the j2ee-server plan. For the J2EE 1.4 configuration, the interop container would be enabled, but if a customer didn't care about the interop container they could remove the plan from the server configuration.
I personally would like to see less plans in Geronimo. I personally find all the plans confusing, but others disagree with me :)
+ /*
+ * Return the skeleton implemention for the remote interface. This interface has the
+ * invoke method to handle the rmi/iiop messages.
+ */
+ public RemoteInterface getRemoteInterface() {
+ if (_ri == null) {
+ synchronized (this) {
+ String riName = _remoteInterfaceName + "_Skeleton";
+ _remoteInterfaceClass = loadClass(riName);
+
+ try {
+ _ri = (RemoteInterface) _remoteInterfaceClass.newInstance();
+ } catch (InstantiationException e) {
+ e.printStackTrace(); //To change body of catch statement use File | Settings | File Templates.
+ } catch (IllegalAccessException e) {
+ e.printStackTrace(); //To change body of catch statement use File | Settings | File Templates.
+ }
+ }
+ }
+
+ return _ri;
+ }
This is double check locking. The whole method needs to be synchronized.
MD>> The double check code is wrong and I need to change it. Does the method require sync? Maybe. I recently came accross a strategy for multiprocessor locking that uses a transient int as a memory barrier. Probably best if I don't attempt to explain it. We can optimize our locking. The template that I came accross is as follows:
private volatile boolean _evaluated = false;
private Object _value;
/**
** Sub-classes should override this method to get the object's value
** when it is first needed.
**/
public abstract Object evaluate();
public final Object getValue() { if (_volatileIsEffectiveMemoryBarrier) { if (! _evaluated) { synchronized (this) { if (! _evaluated) { _value = evaluate(); _evaluated = true; } } } } else { synchronized (this) { if (! _evaluated) { _value = evaluate(); _evaluated = true; } } } return _value; }
Jeremy and I tried a structure like this when we were building locking code for geronimo. The problem we found was evaluating a volatile is like 10 (maybe 100) times slower then entering a synchronized block. It seems that Sun has worked a ton on optimizing synchronized blocked and virtually no work on volatiles. Regardless, I'd go for the easier to read simple synchronize code unless it's on the critical path (and then I'd do some testing :)
Again there is double check locking. Also, if this is called in the critical path, I suggest we use CGLIB instead of reflection for construction as it is way faster.
MD>> I can look into CGLIB.
No rush on that one... it's for the polish phase.
Added: geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ client/InitialContextFactory.java
Url: http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/ java/ org/apache/geronimo/interop/client/InitialContextFactory.java? view=auto&rev=126264
===================================================================== == =======
+public class InitialContextFactory
+ implements javax.naming.spi.InitialContextFactory {
+ private HashMap _startMap = new HashMap();
+
+ public Context getInitialContext(java.util.Hashtable env) throws NamingException {
+ return org.apache.geronimo.interop.rmi.iiop.client.ClientNamingContext.getIn st ance(env);
+ }
+}
Is the _startMap variable used anywhere?
MD>> I'd like to remove any naming in the interop and just use the naming provided in Geronimo.
I just recently re-read the code in geronimo and the code is almost identical. I think the only difference is the implementation in geronimo-naming supports compound name (java:comp/env) lookup from within a context.
