ChangeLogTag:Tue Mar 28 20:53:53 2000 Carlos O'Ryan <coryan@uci.edu>

33 files changed, 351 insertions, 60 deletions

diff --git a/TAO/ChangeLogs/ChangeLog-02a b/TAO/ChangeLogs/ChangeLog-02a
index a1ae6c5801e..7970dd6890a 100644
--- a/TAO/ChangeLogs/ChangeLog-02a
+++ b/TAO/ChangeLogs/ChangeLog-02a
@@ -1,3 +1,26 @@
+Tue Mar 28 20:53:53 2000  Carlos O'Ryan  <coryan@uci.edu>
+
+	* orbsvcs/LoadBalancer/LoadBalancer.h:
+	* orbsvcs/LoadBalancer/LoadBalancer_Service.cpp:
+	* orbsvcs/orbsvcs/LoadBalancing.idl:
+	* orbsvcs/orbsvcs/LoadBalancing/DSI_ForwardingProxy.cpp:
+	* orbsvcs/orbsvcs/LoadBalancing/DSI_ForwardingProxy.h:
+	* orbsvcs/orbsvcs/LoadBalancing/LoadBalancer_i.cpp:
+	* orbsvcs/orbsvcs/LoadBalancing/LoadBalancer_i.h:
+	* orbsvcs/orbsvcs/LoadBalancing/LoadBalancing_Strategy.cpp:
+	* orbsvcs/orbsvcs/LoadBalancing/LoadBalancing_Strategy.h:
+	* orbsvcs/orbsvcs/LoadBalancing/Minimum_Dispersion.cpp:
+	* orbsvcs/orbsvcs/LoadBalancing/Minimum_Dispersion.h:
+	* orbsvcs/orbsvcs/LoadBalancing/ReplicaProxy.cpp:
+	* orbsvcs/orbsvcs/LoadBalancing/ReplicaProxy.h:
+	* orbsvcs/orbsvcs/LoadBalancing/Round_Robin_Strategy.cpp:
+	* orbsvcs/orbsvcs/LoadBalancing/Round_Robin_Strategy.h:
+	* orbsvcs/tests/LoadBalancing/Hash_Replica.idl:
+	* orbsvcs/tests/LoadBalancing/Hash_ReplicaControl.cpp:
+	* orbsvcs/tests/LoadBalancing/Hash_ReplicaControl.h:
+	* orbsvcs/tests/LoadBalancing/client.cpp:
+	  Left a ton of @@ comments for Ossama.
+
 Mon Mar 27 09:03:51 2000  Carlos O'Ryan  <coryan@uci.edu>
 
 	* orbsvcs/tests/LoadBalancing/Hash_ReplicaControl.cpp:
diff --git a/TAO/orbsvcs/LoadBalancer/LoadBalancer.h b/TAO/orbsvcs/LoadBalancer/LoadBalancer.h
index 49418b7fde3..d8e21ac1570 100644
--- a/TAO/orbsvcs/LoadBalancer/LoadBalancer.h
+++ b/TAO/orbsvcs/LoadBalancer/LoadBalancer.h
@@ -56,12 +56,21 @@ private:
   const char *interface_repository_id_;
   // The interface repository ID for the object we load balance.
 
+  // @@ Ossama: the current version just dumps the IOR to a file, we
+  // may want to integrate it with the naming service too.
   const char *load_balancer_file_;
   // The load balancing service IOR is stored in this file
 
   int strategy_;
   // Select the right strategy
 
+  // @@ Ossama: the strategies should be created by a factory, the
+  // factory should be a Service Object that can be dynamically
+  // loaded.
+  // @@ Ossama: here is more food for thought, can we provide a
+  // generic load balancing service that can implement static,
+  // per-request and dynamic load balancing?  If not, can we share the
+  // current strategies among them?
   Round_Robin_Strategy round_robin_;
   Minimum_Dispersion_Strategy minimum_dispersion_;
 
diff --git a/TAO/orbsvcs/LoadBalancer/LoadBalancer_Service.cpp b/TAO/orbsvcs/LoadBalancer/LoadBalancer_Service.cpp
index 9e90d3e4842..d71019e12c9 100644
--- a/TAO/orbsvcs/LoadBalancer/LoadBalancer_Service.cpp
+++ b/TAO/orbsvcs/LoadBalancer/LoadBalancer_Service.cpp
@@ -9,6 +9,7 @@ ACE_RCSID(LoadBalancer, LoadBalancer_Service, "$Id$")
 int
 main (int argc, char *argv[])
 {
+  // @@ Ossama: i'm not sure what the LoadBalancer class buys you.
   ACE_DECLARE_NEW_CORBA_ENV;
   ACE_TRY
     {
diff --git a/TAO/orbsvcs/orbsvcs/DSI_ForwardingProxy.cpp b/TAO/orbsvcs/orbsvcs/DSI_ForwardingProxy.cpp
index d0ad39c9738..17a8628a787 100644
--- a/TAO/orbsvcs/orbsvcs/DSI_ForwardingProxy.cpp
+++ b/TAO/orbsvcs/orbsvcs/DSI_ForwardingProxy.cpp
@@ -12,6 +12,8 @@ DSI_ForwardingProxy::DSI_ForwardingProxy (LoadBalancer_Impl *lb,
   : load_balancer_ (lb), // Hopefully these pointers won't be zero!
     interface_id_ (id)
 {
+  // @@ Ossama: why is this comment useful?
+
   // Nothing else
 }
 
diff --git a/TAO/orbsvcs/orbsvcs/DSI_ForwardingProxy.h b/TAO/orbsvcs/orbsvcs/DSI_ForwardingProxy.h
index bda693bd633..26b2322dd80 100644
--- a/TAO/orbsvcs/orbsvcs/DSI_ForwardingProxy.h
+++ b/TAO/orbsvcs/orbsvcs/DSI_ForwardingProxy.h
@@ -23,6 +23,13 @@
 // Forward declaration.
 class LoadBalancer_Impl;
 
+// @@ Ossama: we have to change the implementation to use a
+// ServantLocator, that can actually raise the ForwardingRequest
+// exception.
+// @@ Also: please remember to use the '#pragma once' madness, prefix
+// the names of your classes and don't forget the TAO_XXXX_Export
+// macros.
+
 class DSI_ForwardingProxy : public PortableServer::DynamicImplementation
 {
   // = TITLE
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancer_i.cpp b/TAO/orbsvcs/orbsvcs/LoadBalancer_i.cpp
index 2e8cd7ee64b..f4dc0bee6a7 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancer_i.cpp
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancer_i.cpp
@@ -18,6 +18,10 @@ LoadBalancer_Impl::LoadBalancer_Impl (const char *interface_id,
     strategy_ (strategy),
     poa_ (PortableServer::POA::_duplicate (poa))
 {
+  // @@ Ossama: in general it is a bad idea to perform operations like
+  // this in the constructor, the behavior on platforms with exception
+  // and without native exception support is simply too different.
+
   // Nothing else
 
   ACE_TRY_NEW_ENV
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancer_i.h b/TAO/orbsvcs/orbsvcs/LoadBalancer_i.h
index 02fc26211bb..11486171bdd 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancer_i.h
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancer_i.h
@@ -23,6 +23,18 @@
 #include "orbsvcs/LoadBalancing/DSI_ForwardingProxy.h"
 #include "orbsvcs/LoadBalancing/LoadBalancing_Strategy.h"
 
+// @@ Ossama: please always remember the #pragma once.
+
+// @@ Ossama: The name of this class should be TAO_XXX_LoadBalancer,
+// or something similar (i vote for XXX == LB)
+// @@ Ossama: the name of the file and the class are inconsistent.
+// @@ Ossama: now is when your idea of keeping the strategy separate
+// pays off: we could add strategies to detect misbehaving replicas,
+// for example, if they don't send any updates in X seconds then they
+// are removed, or maybe we poll them (using _non_existent) before
+// removing.  I'm sure there are other aspects of the system that
+// could be strategized.
+
 class TAO_LoadBalancing_Export LoadBalancer_Impl : public virtual POA_LoadBalancing::LoadBalancer
 {
 public:
@@ -35,27 +47,19 @@ public:
   ~LoadBalancer_Impl (void);
   // Destructor.
 
-  virtual LoadBalancing::ReplicaProxy_ptr connect (
-        LoadBalancing::ReplicaControl_ptr control,
-        CORBA::Object_ptr replica,
-        CORBA::Environment &ACE_TRY_ENV)
-    ACE_THROW_SPEC ((LoadBalancing::ReplicaProxy::NilControl,
-                     LoadBalancing::ReplicaProxy::NilReplica,
-                     CORBA::SystemException));
-  // Register ReplicaControl and Object being load balanced with
-  // ReplicaProxy and connect to Load Balancer.
-
-  virtual CORBA::Object_ptr group_identity (CORBA::Environment &ACE_TRY_ENV)
-    ACE_THROW_SPEC ((CORBA::SystemException));
-  // Return the reference to the object that represents the Replica
-  // group being load balanced.  This "group identity" object will
-  // cause the client to redirect its requests to a Replica that fits
-  // a specific load balancing criteria.
-
   // Local methods
 
+  // @@ Ossama: this method is something of my own invention, but i
+  // think we need to change the interface, to something like:
+  //    load_changed (float old_load, float new_load, proxy)
+  // that should let us keep the average load (and other similar
+  // things) pre-computed.
+  //
   void load_changed (ReplicaProxy_Impl *proxy,
                      CORBA::Environment &ACE_TRY_ENV);
+  // The load for <proxy> has changed, the LoadBalancer can use this
+  // opportunity to determine if the load on one of the services is
+  // too high.
 
   void disconnect (ReplicaProxy_Impl *proxy,
                    CORBA::Environment &ACE_TRY_ENV)
@@ -67,6 +71,17 @@ public:
   // Return a reference to the Replica to which client requests should
   // be redirected next.
 
+  // = See the descriptions in LoadBalancing.idl
+  virtual LoadBalancing::ReplicaProxy_ptr connect (
+        LoadBalancing::ReplicaControl_ptr control,
+        CORBA::Object_ptr replica,
+        CORBA::Environment &ACE_TRY_ENV)
+    ACE_THROW_SPEC ((LoadBalancing::ReplicaProxy::NilControl,
+                     LoadBalancing::ReplicaProxy::NilReplica,
+                     CORBA::SystemException));
+  virtual CORBA::Object_ptr group_identity (CORBA::Environment &ACE_TRY_ENV)
+    ACE_THROW_SPEC ((CORBA::SystemException));
+
 private:
   DSI_ForwardingProxy redirector_;
   // The object that tells the invoking client to forward its requests
@@ -84,6 +99,7 @@ private:
 
 #if defined (__ACE_INLINE__)
 #include "LoadBalancer_i.i"
+// @@ Ossama: notice that i use relative paths!
 #endif /* __ACE_INLINE__ */
 
 #endif  /* TAO_LOADBALANCER_I_H */
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing.idl b/TAO/orbsvcs/orbsvcs/LoadBalancing.idl
index 263c37d1521..817726535f8 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing.idl
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing.idl
@@ -4,7 +4,7 @@
 // ============================================================================
 //
 // = LIBRARY
-//    orbsvcs
+//   TAO_LoadBalancing
 //
 // = FILENAME
 //    LoadBalancing.idl
@@ -20,13 +20,19 @@
 module LoadBalancing
 {
   // = TITLE
-  //    This module provides TAO's Load Balancer interface.
+  //   This module defines the interfaces and data types used in TAO's
+  //   Load Balancing service.
 
   // = DESCRIPTION
   //     TAO's Load Balancer manages distribution of requests to
   //     replicas of a given Object in an effort to ensure that the
   //     applications/hosts pointed to by the Object reference are as
   //     equally loaded as possible, i.e. load balanced.
+  //     The definition of 'load' is application specific, for
+  //     example, some applications may choose to load balance access
+  //     to multiple dedicated lines, or separate network interfaces,
+  //     as well as more traditional load balancing metrics, such as
+  //     CPU or disk load.
 
   interface ReplicaControl
     {
@@ -96,10 +102,11 @@ module LoadBalancing
         };
 
       oneway void current_load (in float load);
-      // Send current load to the Load Balancer where the definition
-      // of "load" is load balancing algorithm specific.
+      // Send current load to the Load Balancer.
+      // The application must ensure that all replicas use the same
+      // notion of load.
 
-      void disconnect ();
+      void disconnect () raises (NotConnected);
       // Disconnect the ReplicaControl from the Load Balancer.  The
       // Object that the ReplicaControl controls will be no longer be
       // load balanced.
@@ -137,6 +144,17 @@ module LoadBalancing
       // object will cause the client to redirect its requests to a
       // Replica that fits a specific load balancing criteria.
     };
+
+  // @@ Ossama: we may want to add interfaces or operations to access
+  // the current list of loads, that would be useful for monitoring
+  // applications, and nice GUI-based demos.
+  // @@ Ossama: another idea: we have been using this stuff to make
+  // all the loads equal, but what if the objective is to keep the
+  // load below some value?  Maybe we should provide some callback
+  // mechanism to let the application know: the load is too high and
+  // there is nothing i can do about it, add more CPUS or reduce the
+  // load!
+  // Such feedback would be very useful for some applications.
 };
 
 #endif /* TAO_LOADBALANCER_IDL */
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/DSI_ForwardingProxy.cpp b/TAO/orbsvcs/orbsvcs/LoadBalancing/DSI_ForwardingProxy.cpp
index d0ad39c9738..17a8628a787 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/DSI_ForwardingProxy.cpp
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/DSI_ForwardingProxy.cpp
@@ -12,6 +12,8 @@ DSI_ForwardingProxy::DSI_ForwardingProxy (LoadBalancer_Impl *lb,
   : load_balancer_ (lb), // Hopefully these pointers won't be zero!
     interface_id_ (id)
 {
+  // @@ Ossama: why is this comment useful?
+
   // Nothing else
 }
 
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/DSI_ForwardingProxy.h b/TAO/orbsvcs/orbsvcs/LoadBalancing/DSI_ForwardingProxy.h
index bda693bd633..26b2322dd80 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/DSI_ForwardingProxy.h
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/DSI_ForwardingProxy.h
@@ -23,6 +23,13 @@
 // Forward declaration.
 class LoadBalancer_Impl;
 
+// @@ Ossama: we have to change the implementation to use a
+// ServantLocator, that can actually raise the ForwardingRequest
+// exception.
+// @@ Also: please remember to use the '#pragma once' madness, prefix
+// the names of your classes and don't forget the TAO_XXXX_Export
+// macros.
+
 class DSI_ForwardingProxy : public PortableServer::DynamicImplementation
 {
   // = TITLE
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancer_i.cpp b/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancer_i.cpp
index 2e8cd7ee64b..f4dc0bee6a7 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancer_i.cpp
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancer_i.cpp
@@ -18,6 +18,10 @@ LoadBalancer_Impl::LoadBalancer_Impl (const char *interface_id,
     strategy_ (strategy),
     poa_ (PortableServer::POA::_duplicate (poa))
 {
+  // @@ Ossama: in general it is a bad idea to perform operations like
+  // this in the constructor, the behavior on platforms with exception
+  // and without native exception support is simply too different.
+
   // Nothing else
 
   ACE_TRY_NEW_ENV
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancer_i.h b/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancer_i.h
index 02fc26211bb..11486171bdd 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancer_i.h
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancer_i.h
@@ -23,6 +23,18 @@
 #include "orbsvcs/LoadBalancing/DSI_ForwardingProxy.h"
 #include "orbsvcs/LoadBalancing/LoadBalancing_Strategy.h"
 
+// @@ Ossama: please always remember the #pragma once.
+
+// @@ Ossama: The name of this class should be TAO_XXX_LoadBalancer,
+// or something similar (i vote for XXX == LB)
+// @@ Ossama: the name of the file and the class are inconsistent.
+// @@ Ossama: now is when your idea of keeping the strategy separate
+// pays off: we could add strategies to detect misbehaving replicas,
+// for example, if they don't send any updates in X seconds then they
+// are removed, or maybe we poll them (using _non_existent) before
+// removing.  I'm sure there are other aspects of the system that
+// could be strategized.
+
 class TAO_LoadBalancing_Export LoadBalancer_Impl : public virtual POA_LoadBalancing::LoadBalancer
 {
 public:
@@ -35,27 +47,19 @@ public:
   ~LoadBalancer_Impl (void);
   // Destructor.
 
-  virtual LoadBalancing::ReplicaProxy_ptr connect (
-        LoadBalancing::ReplicaControl_ptr control,
-        CORBA::Object_ptr replica,
-        CORBA::Environment &ACE_TRY_ENV)
-    ACE_THROW_SPEC ((LoadBalancing::ReplicaProxy::NilControl,
-                     LoadBalancing::ReplicaProxy::NilReplica,
-                     CORBA::SystemException));
-  // Register ReplicaControl and Object being load balanced with
-  // ReplicaProxy and connect to Load Balancer.
-
-  virtual CORBA::Object_ptr group_identity (CORBA::Environment &ACE_TRY_ENV)
-    ACE_THROW_SPEC ((CORBA::SystemException));
-  // Return the reference to the object that represents the Replica
-  // group being load balanced.  This "group identity" object will
-  // cause the client to redirect its requests to a Replica that fits
-  // a specific load balancing criteria.
-
   // Local methods
 
+  // @@ Ossama: this method is something of my own invention, but i
+  // think we need to change the interface, to something like:
+  //    load_changed (float old_load, float new_load, proxy)
+  // that should let us keep the average load (and other similar
+  // things) pre-computed.
+  //
   void load_changed (ReplicaProxy_Impl *proxy,
                      CORBA::Environment &ACE_TRY_ENV);
+  // The load for <proxy> has changed, the LoadBalancer can use this
+  // opportunity to determine if the load on one of the services is
+  // too high.
 
   void disconnect (ReplicaProxy_Impl *proxy,
                    CORBA::Environment &ACE_TRY_ENV)
@@ -67,6 +71,17 @@ public:
   // Return a reference to the Replica to which client requests should
   // be redirected next.
 
+  // = See the descriptions in LoadBalancing.idl
+  virtual LoadBalancing::ReplicaProxy_ptr connect (
+        LoadBalancing::ReplicaControl_ptr control,
+        CORBA::Object_ptr replica,
+        CORBA::Environment &ACE_TRY_ENV)
+    ACE_THROW_SPEC ((LoadBalancing::ReplicaProxy::NilControl,
+                     LoadBalancing::ReplicaProxy::NilReplica,
+                     CORBA::SystemException));
+  virtual CORBA::Object_ptr group_identity (CORBA::Environment &ACE_TRY_ENV)
+    ACE_THROW_SPEC ((CORBA::SystemException));
+
 private:
   DSI_ForwardingProxy redirector_;
   // The object that tells the invoking client to forward its requests
@@ -84,6 +99,7 @@ private:
 
 #if defined (__ACE_INLINE__)
 #include "LoadBalancer_i.i"
+// @@ Ossama: notice that i use relative paths!
 #endif /* __ACE_INLINE__ */
 
 #endif  /* TAO_LOADBALANCER_I_H */
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancing_Strategy.cpp b/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancing_Strategy.cpp
index 1158d835919..bb90a27e7c8 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancing_Strategy.cpp
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancing_Strategy.cpp
@@ -22,6 +22,8 @@ Load_Balancing_Strategy::load_changed (ReplicaProxy_Impl *,
 {
 }
 
+// @@ Ossama: i had to instantiate the template here because it is
+// used in two strategies, but it sound kind of silly...
 #if defined (ACE_HAS_EXPLICIT_TEMPLATE_INSTANTIATION)
 
 template class ACE_Node<ReplicaProxy_Impl*>;
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancing_Strategy.h b/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancing_Strategy.h
index 3daa2a7e8e3..72b580e6919 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancing_Strategy.h
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/LoadBalancing_Strategy.h
@@ -20,6 +20,9 @@
 
 #include "orbsvcs/LoadBalancingS.h"
 
+// @@ Ossama: the general comments on filenames, class names,
+// TAO_XXX_Export an other things like that apply.
+
 class ReplicaProxy_Impl;
 
 // The abstract load balancing strategy class.
@@ -37,6 +40,9 @@ public:
   virtual ~Load_Balancing_Strategy (void);
   // Destructor
 
+  // @@ The name of the method should be more meaningful, what about:
+  //    select_replica(), select_unloaded_replica() or something like
+  //    that?
   virtual CORBA::Object_ptr replica (CORBA::Environment &ACE_TRY_ENV)
     ACE_THROW_SPEC ((CORBA::SystemException)) = 0;
   // Return the object reference to the Replica to which requests should
@@ -52,6 +58,8 @@ public:
   virtual void load_changed (ReplicaProxy_Impl *impl,
                              CORBA::Environment &ACE_TRY_ENV);
   // The load on one proxy has changed.
+  // @@ Ossama: see my comments on LoadBalancing_i about changing the
+  // signature of this method.
 };
 
 #if !defined (__ACE_INLINE__)
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/Minimum_Dispersion.cpp b/TAO/orbsvcs/orbsvcs/LoadBalancing/Minimum_Dispersion.cpp
index f02daeb1f77..a7c758be4cb 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/Minimum_Dispersion.cpp
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/Minimum_Dispersion.cpp
@@ -16,6 +16,7 @@ Minimum_Dispersion_Strategy::Minimum_Dispersion_Strategy (void)
 
 Minimum_Dispersion_Strategy::~Minimum_Dispersion_Strategy (void)
 {
+  // @@ Ossama: more code that is not thread safe
   ReplicaProxySetIterator begin = this->proxies_.begin ();
   ReplicaProxySetIterator end = this->proxies_.end ();
 
@@ -31,6 +32,7 @@ Minimum_Dispersion_Strategy::~Minimum_Dispersion_Strategy (void)
 CORBA::Object_ptr
 Minimum_Dispersion_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
 {
+  // @@ Ossama: more code that is not thread safe
   while (!this->proxies_.is_empty ())
     {
       ReplicaProxySetIterator begin = this->proxies_.begin ();
@@ -49,9 +51,11 @@ Minimum_Dispersion_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
             }
         }
 
-      // @@ Setup a timeout
+      // @@ Ossama: we should setup a timeout policy here...
       ACE_TRY
         {
+          // Before returning an object reference to the client
+          // validate it first.
           CORBA::Object_var object =
             proxy->replica ();
           CORBA::Boolean non_existent =
@@ -66,7 +70,11 @@ Minimum_Dispersion_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
         {
         }
       ACE_ENDTRY;
-      // @@ A bit melodramatic...
+      // @@ Ossama: a bit melodramatic, we remove the object if *any*
+      // exception is thrown.  If the object really does not exist (we
+      // get non_existent==1) then this is exactly what we want to do,
+      // but if we get something like TRANSIENT we may want to do
+      // something less drastic, or at least strategize it ;-)
       this->proxies_.remove (proxy);
     }
   // @@ What do we do if the set is empty?
@@ -78,12 +86,14 @@ Minimum_Dispersion_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
 int
 Minimum_Dispersion_Strategy::insert (ReplicaProxy_Impl *proxy)
 {
+  // @@ Ossama: more code that is not thread safe
   return this->proxies_.insert (proxy);
 }
 
 int
 Minimum_Dispersion_Strategy::remove (ReplicaProxy_Impl *proxy)
 {
+  // @@ Ossama: more code that is not thread safe
   return this->proxies_.remove (proxy);
 }
 
@@ -91,6 +101,7 @@ void
 Minimum_Dispersion_Strategy::load_changed (ReplicaProxy_Impl *proxy,
                                            CORBA::Environment &ACE_TRY_ENV)
 {
+  // @@ Ossama: more code that is not thread safe
   if (this->proxies_.is_empty ())
     return;
 
@@ -114,9 +125,12 @@ Minimum_Dispersion_Strategy::load_changed (ReplicaProxy_Impl *proxy,
 
   float relative_load = cl / avg;
 
+  // @@ Ossama: no debug messages in production code, my fault....
   ACE_DEBUG ((LM_DEBUG, "Load[%x] %f %f %f\n",
               proxy, cl, avg, relative_load));
 
+  // @@ Ossama: Make the 1.5 factor adjustable, it is how much
+  // dispersion we tolerate before starting to send advisories.
   if (relative_load > 1 + 1.5F / n)
     {
       proxy->has_high_load_ = 1;
@@ -125,6 +139,12 @@ Minimum_Dispersion_Strategy::load_changed (ReplicaProxy_Impl *proxy,
       return;
     }
 
+  // @@ Ossama: notice that we wait until the load is signifcantly
+  // lower before sending the nominal load advisory, it does not
+  // matter that much because the replicas automatically restart after
+  // rejecting one client....
+  // @@ Ossama: make the 0.9 factor adjustable, at least at
+  // construction time...
   if (proxy->has_high_load_ && relative_load < 1 + 0.9F / n)
     {
       proxy->has_high_load_ = 0;
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/Minimum_Dispersion.h b/TAO/orbsvcs/orbsvcs/LoadBalancing/Minimum_Dispersion.h
index a5f84193e77..213eb22b443 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/Minimum_Dispersion.h
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/Minimum_Dispersion.h
@@ -22,6 +22,11 @@
 #include "orbsvcs/LoadBalancingS.h"
 #include "ace/Containers.h"
 
+// @@ Ossama: my class names suck too, any idea on what would be a
+// good name for this strategy? It basically tries to keep all the
+// loads "close to the average",  if one server is much higher than
+// the load it is deemed too loaded and the advisory is sent.
+
 // Forward declarations
 class ReplicaProxy_Impl;
 
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/ReplicaProxy.cpp b/TAO/orbsvcs/orbsvcs/LoadBalancing/ReplicaProxy.cpp
index 42d4f9f7121..1b8ce2171cb 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/ReplicaProxy.cpp
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/ReplicaProxy.cpp
@@ -26,6 +26,9 @@ ReplicaProxy_Impl::current_load (CORBA::Float load,
   ACE_THROW_SPEC ((LoadBalancing::ReplicaProxy::InvalidLoad,
                    CORBA::SystemException))
 {
+  // @@ Ossama: this is the point were the load dampening should
+  // happen. Probably strategized....
+
   if (load < 0)
     ACE_THROW (LoadBalancing::ReplicaProxy::InvalidLoad ());
 
@@ -40,6 +43,8 @@ ReplicaProxy_Impl::disconnect (CORBA::Environment &ACE_TRY_ENV)
   ACE_THROW_SPEC ((LoadBalancing::ReplicaProxy::NotConnected,
                    CORBA::SystemException))
 {
+  // @@ Ossama: this code is not thread safe...
+
   if (this->connected_)
     {
       this->balancer_->disconnect (this, ACE_TRY_ENV);
@@ -61,6 +66,11 @@ void ReplicaProxy_Impl::connect (LoadBalancer_Impl *balancer,
                    LoadBalancing::ReplicaProxy::NilReplica,
                    CORBA::SystemException))
 {
+  // @@ Ossama: this is a perfect example of code that is not thread
+  // safe: what if we get a 'current_load' message in another thread?
+  // Or a disconnect() message from a misbehaving replica? Or two
+  // calls to connect?
+
   if (balancer == 0)
     ACE_THROW (CORBA::BAD_PARAM (
       CORBA_SystemException::_tao_minor_code (
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/ReplicaProxy.h b/TAO/orbsvcs/orbsvcs/LoadBalancing/ReplicaProxy.h
index ef297cda029..fa4679be71e 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/ReplicaProxy.h
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/ReplicaProxy.h
@@ -39,6 +39,13 @@ public:
 
   ReplicaProxy_Impl (void);
 
+  CORBA::Float current_load (void) const;
+  // Read the stored load of the Object being load balanced from the
+  // ReplicaProxy.
+
+  CORBA::Object_ptr replica (void);
+  // Return a reference to the Replica.
+
   virtual void current_load (CORBA::Float load,
                              CORBA::Environment &ACE_TRY_ENV)
     ACE_THROW_SPEC ((LoadBalancing::ReplicaProxy::InvalidLoad,
@@ -46,20 +53,25 @@ public:
   // Send the value of the current load on the Object being load
   // balanced, where the definition of "load" is load balancing
   // algorithm specific.
-
-  CORBA::Float current_load (void) const;
-  // Read the stored load of the Object being load balanced from the
-  // ReplicaProxy.
+  // @@ Ossama: i had to implement dampening mechanisms in the load,
+  // otherwise the spikes where killing me, but i'm not sure if that
+  // should be implemented in the client or the server.
+  // @@ I think that the 'Right Thing' is to have the client send
+  // 'instantaneous load' and get the load balancing service to do the
+  // dampening... that way we can modify the algorithm in the server,
+  // without affecting the client, or detect rapid raises in load and
+  // react proactively to those...
 
   virtual void disconnect (CORBA::Environment &ACE_TRY_ENV)
     ACE_THROW_SPEC ((CORBA::SystemException));
   // Send a request to disconnect from the LoadBalancer.
 
-  CORBA::Object_ptr replica (void);
-  // Return a reference to the Replica.
-
+  // @@ Ossama: could you please fix the mess i left here? it is my
+  // fault, but i was running short on time.
   int has_high_load_;
   // Has the high load advisory been sent
+  // @@ Ossama: i added this flag to avoid multiple nomimal load
+  // advisories and high load advisories to be sent.
 
   LoadBalancing::ReplicaControl_var control_;
   // Reference to the ReplicaControl.
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/Round_Robin_Strategy.cpp b/TAO/orbsvcs/orbsvcs/LoadBalancing/Round_Robin_Strategy.cpp
index 5f21b39a804..4b995d286e8 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/Round_Robin_Strategy.cpp
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/Round_Robin_Strategy.cpp
@@ -17,6 +17,10 @@ Round_Robin_Strategy::Round_Robin_Strategy (void)
 
 Round_Robin_Strategy::~Round_Robin_Strategy (void)
 {
+  // @@ Are the objects deactivated from the POA?  And shouldn't this
+  // be done by the LoadBalancing strategy *before* the destructor is
+  // invoked?
+
   ReplicaProxySetIterator begin = this->proxies_.begin ();
   ReplicaProxySetIterator end = this->proxies_.end ();
 
@@ -32,9 +36,14 @@ Round_Robin_Strategy::~Round_Robin_Strategy (void)
 CORBA::Object_ptr
 Round_Robin_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
 {
+  // @@ Ossama: more code that is not thread safe
   if (this->proxies_.is_empty ())
     {
       // @@ What do we do if the set is empty?
+      // @@ Ossama: i'm throwing the OBJECT_NOT_EXIST exception, but
+      // maybe TRANSIENT is a better choice.... please read the
+      // relevant section of the CORBA spec and let me know what you
+      // think.
       ACE_THROW_RETURN (CORBA::OBJECT_NOT_EXIST (),
                         CORBA::Object::_nil ());
     }
@@ -50,12 +59,24 @@ Round_Robin_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
   // next call to this method.
   this->next_replica_++;
 
+  // @@ Ossama: once we select a replica, shouldn't we temporarily
+  // change its load so it does not get swamped with all the new
+  // clients. Think about the current scenario:
+  // - Replica A has the lowest load, by far.
+  // - 5 clients connect at the same time.
+  // - If the load on A does not grow fast enough all 5 clients will
+  // end up on replica A, and then they will be kicked out and
+  // shuffled around.
+  // While if we bump the load for a few seconds then the next client
+  // will go to other place.  Or set a flag that says 'load
+  // invalidated, waiting for an update' (i like the latter better).
   return proxy_servant->replica ();
 }
 
 int
 Round_Robin_Strategy::insert (ReplicaProxy_Impl *proxy)
 {
+  // @@ Ossama: more code that is not thread safe
   int r = this->proxies_.insert (proxy);
   this->next_replica_ = this->proxies_.begin ();
   return r;
@@ -64,6 +85,7 @@ Round_Robin_Strategy::insert (ReplicaProxy_Impl *proxy)
 int
 Round_Robin_Strategy::remove (ReplicaProxy_Impl *proxy)
 {
+  // @@ Ossama: more code that is not thread safe
   int r = this->proxies_.remove (proxy);
   this->next_replica_ = this->proxies_.begin ();
   return r;
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing/Round_Robin_Strategy.h b/TAO/orbsvcs/orbsvcs/LoadBalancing/Round_Robin_Strategy.h
index e65d8cdb39c..a279071a46f 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing/Round_Robin_Strategy.h
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing/Round_Robin_Strategy.h
@@ -25,6 +25,13 @@
 // Forward declarations
 class ReplicaProxy_Impl;
 
+// @@ Ossama: is this the kind of data structure that you want to use
+// in this case? I mean the iterator is invalidated on each insertion
+// and extraction, the class is not thread safe and it is hard to find
+// the element with the least load...  Then again the number of
+// servers should be small (say around 10), and additions/removals
+// rare, so the data structure is probably not a real problem.
+
 typedef ACE_Unbounded_Set<ReplicaProxy_Impl *> ReplicaProxySet;
 typedef ACE_Unbounded_Set_Iterator<ReplicaProxy_Impl *> ReplicaProxySetIterator;
 
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing_Strategy.cpp b/TAO/orbsvcs/orbsvcs/LoadBalancing_Strategy.cpp
index 1158d835919..bb90a27e7c8 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing_Strategy.cpp
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing_Strategy.cpp
@@ -22,6 +22,8 @@ Load_Balancing_Strategy::load_changed (ReplicaProxy_Impl *,
 {
 }
 
+// @@ Ossama: i had to instantiate the template here because it is
+// used in two strategies, but it sound kind of silly...
 #if defined (ACE_HAS_EXPLICIT_TEMPLATE_INSTANTIATION)
 
 template class ACE_Node<ReplicaProxy_Impl*>;
diff --git a/TAO/orbsvcs/orbsvcs/LoadBalancing_Strategy.h b/TAO/orbsvcs/orbsvcs/LoadBalancing_Strategy.h
index 3daa2a7e8e3..72b580e6919 100644
--- a/TAO/orbsvcs/orbsvcs/LoadBalancing_Strategy.h
+++ b/TAO/orbsvcs/orbsvcs/LoadBalancing_Strategy.h
@@ -20,6 +20,9 @@
 
 #include "orbsvcs/LoadBalancingS.h"
 
+// @@ Ossama: the general comments on filenames, class names,
+// TAO_XXX_Export an other things like that apply.
+
 class ReplicaProxy_Impl;
 
 // The abstract load balancing strategy class.
@@ -37,6 +40,9 @@ public:
   virtual ~Load_Balancing_Strategy (void);
   // Destructor
 
+  // @@ The name of the method should be more meaningful, what about:
+  //    select_replica(), select_unloaded_replica() or something like
+  //    that?
   virtual CORBA::Object_ptr replica (CORBA::Environment &ACE_TRY_ENV)
     ACE_THROW_SPEC ((CORBA::SystemException)) = 0;
   // Return the object reference to the Replica to which requests should
@@ -52,6 +58,8 @@ public:
   virtual void load_changed (ReplicaProxy_Impl *impl,
                              CORBA::Environment &ACE_TRY_ENV);
   // The load on one proxy has changed.
+  // @@ Ossama: see my comments on LoadBalancing_i about changing the
+  // signature of this method.
 };
 
 #if !defined (__ACE_INLINE__)
diff --git a/TAO/orbsvcs/orbsvcs/Makefile.LoadBalancing b/TAO/orbsvcs/orbsvcs/Makefile.LoadBalancing
index 85af9c32256..53cc28d34ac 100644
--- a/TAO/orbsvcs/orbsvcs/Makefile.LoadBalancing
+++ b/TAO/orbsvcs/orbsvcs/Makefile.LoadBalancing
@@ -61,7 +61,7 @@ include $(ACE_ROOT)/include/makeinclude/rules.local.GNU
 #       Local targets (and local hacks)
 #----------------------------------------------------------------------------
 
-LDFLAGS += -L$(TAO_ROOT)/tao
+LDFLAGS += -L$(TAO_ROOT)/tao -L$(TAO_ROOT)/orbsvcs/orbsvcs
 CPPFLAGS += -I$(TAO_ROOT) -I$(TAO_ROOT)/orbsvcs
 
 .PRECIOUS: $(foreach file, $(IDL_SRCS), $(foreach ext, $(IDL_EXT), $(file)$(ext))))
diff --git a/TAO/orbsvcs/orbsvcs/Minimum_Dispersion.cpp b/TAO/orbsvcs/orbsvcs/Minimum_Dispersion.cpp
index f02daeb1f77..a7c758be4cb 100644
--- a/TAO/orbsvcs/orbsvcs/Minimum_Dispersion.cpp
+++ b/TAO/orbsvcs/orbsvcs/Minimum_Dispersion.cpp
@@ -16,6 +16,7 @@ Minimum_Dispersion_Strategy::Minimum_Dispersion_Strategy (void)
 
 Minimum_Dispersion_Strategy::~Minimum_Dispersion_Strategy (void)
 {
+  // @@ Ossama: more code that is not thread safe
   ReplicaProxySetIterator begin = this->proxies_.begin ();
   ReplicaProxySetIterator end = this->proxies_.end ();
 
@@ -31,6 +32,7 @@ Minimum_Dispersion_Strategy::~Minimum_Dispersion_Strategy (void)
 CORBA::Object_ptr
 Minimum_Dispersion_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
 {
+  // @@ Ossama: more code that is not thread safe
   while (!this->proxies_.is_empty ())
     {
       ReplicaProxySetIterator begin = this->proxies_.begin ();
@@ -49,9 +51,11 @@ Minimum_Dispersion_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
             }
         }
 
-      // @@ Setup a timeout
+      // @@ Ossama: we should setup a timeout policy here...
       ACE_TRY
         {
+          // Before returning an object reference to the client
+          // validate it first.
           CORBA::Object_var object =
             proxy->replica ();
           CORBA::Boolean non_existent =
@@ -66,7 +70,11 @@ Minimum_Dispersion_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
         {
         }
       ACE_ENDTRY;
-      // @@ A bit melodramatic...
+      // @@ Ossama: a bit melodramatic, we remove the object if *any*
+      // exception is thrown.  If the object really does not exist (we
+      // get non_existent==1) then this is exactly what we want to do,
+      // but if we get something like TRANSIENT we may want to do
+      // something less drastic, or at least strategize it ;-)
       this->proxies_.remove (proxy);
     }
   // @@ What do we do if the set is empty?
@@ -78,12 +86,14 @@ Minimum_Dispersion_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
 int
 Minimum_Dispersion_Strategy::insert (ReplicaProxy_Impl *proxy)
 {
+  // @@ Ossama: more code that is not thread safe
   return this->proxies_.insert (proxy);
 }
 
 int
 Minimum_Dispersion_Strategy::remove (ReplicaProxy_Impl *proxy)
 {
+  // @@ Ossama: more code that is not thread safe
   return this->proxies_.remove (proxy);
 }
 
@@ -91,6 +101,7 @@ void
 Minimum_Dispersion_Strategy::load_changed (ReplicaProxy_Impl *proxy,
                                            CORBA::Environment &ACE_TRY_ENV)
 {
+  // @@ Ossama: more code that is not thread safe
   if (this->proxies_.is_empty ())
     return;
 
@@ -114,9 +125,12 @@ Minimum_Dispersion_Strategy::load_changed (ReplicaProxy_Impl *proxy,
 
   float relative_load = cl / avg;
 
+  // @@ Ossama: no debug messages in production code, my fault....
   ACE_DEBUG ((LM_DEBUG, "Load[%x] %f %f %f\n",
               proxy, cl, avg, relative_load));
 
+  // @@ Ossama: Make the 1.5 factor adjustable, it is how much
+  // dispersion we tolerate before starting to send advisories.
   if (relative_load > 1 + 1.5F / n)
     {
       proxy->has_high_load_ = 1;
@@ -125,6 +139,12 @@ Minimum_Dispersion_Strategy::load_changed (ReplicaProxy_Impl *proxy,
       return;
     }
 
+  // @@ Ossama: notice that we wait until the load is signifcantly
+  // lower before sending the nominal load advisory, it does not
+  // matter that much because the replicas automatically restart after
+  // rejecting one client....
+  // @@ Ossama: make the 0.9 factor adjustable, at least at
+  // construction time...
   if (proxy->has_high_load_ && relative_load < 1 + 0.9F / n)
     {
       proxy->has_high_load_ = 0;
diff --git a/TAO/orbsvcs/orbsvcs/Minimum_Dispersion.h b/TAO/orbsvcs/orbsvcs/Minimum_Dispersion.h
index a5f84193e77..213eb22b443 100644
--- a/TAO/orbsvcs/orbsvcs/Minimum_Dispersion.h
+++ b/TAO/orbsvcs/orbsvcs/Minimum_Dispersion.h
@@ -22,6 +22,11 @@
 #include "orbsvcs/LoadBalancingS.h"
 #include "ace/Containers.h"
 
+// @@ Ossama: my class names suck too, any idea on what would be a
+// good name for this strategy? It basically tries to keep all the
+// loads "close to the average",  if one server is much higher than
+// the load it is deemed too loaded and the advisory is sent.
+
 // Forward declarations
 class ReplicaProxy_Impl;
 
diff --git a/TAO/orbsvcs/orbsvcs/ReplicaProxy.cpp b/TAO/orbsvcs/orbsvcs/ReplicaProxy.cpp
index 42d4f9f7121..1b8ce2171cb 100644
--- a/TAO/orbsvcs/orbsvcs/ReplicaProxy.cpp
+++ b/TAO/orbsvcs/orbsvcs/ReplicaProxy.cpp
@@ -26,6 +26,9 @@ ReplicaProxy_Impl::current_load (CORBA::Float load,
   ACE_THROW_SPEC ((LoadBalancing::ReplicaProxy::InvalidLoad,
                    CORBA::SystemException))
 {
+  // @@ Ossama: this is the point were the load dampening should
+  // happen. Probably strategized....
+
   if (load < 0)
     ACE_THROW (LoadBalancing::ReplicaProxy::InvalidLoad ());
 
@@ -40,6 +43,8 @@ ReplicaProxy_Impl::disconnect (CORBA::Environment &ACE_TRY_ENV)
   ACE_THROW_SPEC ((LoadBalancing::ReplicaProxy::NotConnected,
                    CORBA::SystemException))
 {
+  // @@ Ossama: this code is not thread safe...
+
   if (this->connected_)
     {
       this->balancer_->disconnect (this, ACE_TRY_ENV);
@@ -61,6 +66,11 @@ void ReplicaProxy_Impl::connect (LoadBalancer_Impl *balancer,
                    LoadBalancing::ReplicaProxy::NilReplica,
                    CORBA::SystemException))
 {
+  // @@ Ossama: this is a perfect example of code that is not thread
+  // safe: what if we get a 'current_load' message in another thread?
+  // Or a disconnect() message from a misbehaving replica? Or two
+  // calls to connect?
+
   if (balancer == 0)
     ACE_THROW (CORBA::BAD_PARAM (
       CORBA_SystemException::_tao_minor_code (
diff --git a/TAO/orbsvcs/orbsvcs/ReplicaProxy.h b/TAO/orbsvcs/orbsvcs/ReplicaProxy.h
index ef297cda029..fa4679be71e 100644
--- a/TAO/orbsvcs/orbsvcs/ReplicaProxy.h
+++ b/TAO/orbsvcs/orbsvcs/ReplicaProxy.h
@@ -39,6 +39,13 @@ public:
 
   ReplicaProxy_Impl (void);
 
+  CORBA::Float current_load (void) const;
+  // Read the stored load of the Object being load balanced from the
+  // ReplicaProxy.
+
+  CORBA::Object_ptr replica (void);
+  // Return a reference to the Replica.
+
   virtual void current_load (CORBA::Float load,
                              CORBA::Environment &ACE_TRY_ENV)
     ACE_THROW_SPEC ((LoadBalancing::ReplicaProxy::InvalidLoad,
@@ -46,20 +53,25 @@ public:
   // Send the value of the current load on the Object being load
   // balanced, where the definition of "load" is load balancing
   // algorithm specific.
-
-  CORBA::Float current_load (void) const;
-  // Read the stored load of the Object being load balanced from the
-  // ReplicaProxy.
+  // @@ Ossama: i had to implement dampening mechanisms in the load,
+  // otherwise the spikes where killing me, but i'm not sure if that
+  // should be implemented in the client or the server.
+  // @@ I think that the 'Right Thing' is to have the client send
+  // 'instantaneous load' and get the load balancing service to do the
+  // dampening... that way we can modify the algorithm in the server,
+  // without affecting the client, or detect rapid raises in load and
+  // react proactively to those...
 
   virtual void disconnect (CORBA::Environment &ACE_TRY_ENV)
     ACE_THROW_SPEC ((CORBA::SystemException));
   // Send a request to disconnect from the LoadBalancer.
 
-  CORBA::Object_ptr replica (void);
-  // Return a reference to the Replica.
-
+  // @@ Ossama: could you please fix the mess i left here? it is my
+  // fault, but i was running short on time.
   int has_high_load_;
   // Has the high load advisory been sent
+  // @@ Ossama: i added this flag to avoid multiple nomimal load
+  // advisories and high load advisories to be sent.
 
   LoadBalancing::ReplicaControl_var control_;
   // Reference to the ReplicaControl.
diff --git a/TAO/orbsvcs/orbsvcs/Round_Robin_Strategy.cpp b/TAO/orbsvcs/orbsvcs/Round_Robin_Strategy.cpp
index 5f21b39a804..4b995d286e8 100644
--- a/TAO/orbsvcs/orbsvcs/Round_Robin_Strategy.cpp
+++ b/TAO/orbsvcs/orbsvcs/Round_Robin_Strategy.cpp
@@ -17,6 +17,10 @@ Round_Robin_Strategy::Round_Robin_Strategy (void)
 
 Round_Robin_Strategy::~Round_Robin_Strategy (void)
 {
+  // @@ Are the objects deactivated from the POA?  And shouldn't this
+  // be done by the LoadBalancing strategy *before* the destructor is
+  // invoked?
+
   ReplicaProxySetIterator begin = this->proxies_.begin ();
   ReplicaProxySetIterator end = this->proxies_.end ();
 
@@ -32,9 +36,14 @@ Round_Robin_Strategy::~Round_Robin_Strategy (void)
 CORBA::Object_ptr
 Round_Robin_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
 {
+  // @@ Ossama: more code that is not thread safe
   if (this->proxies_.is_empty ())
     {
       // @@ What do we do if the set is empty?
+      // @@ Ossama: i'm throwing the OBJECT_NOT_EXIST exception, but
+      // maybe TRANSIENT is a better choice.... please read the
+      // relevant section of the CORBA spec and let me know what you
+      // think.
       ACE_THROW_RETURN (CORBA::OBJECT_NOT_EXIST (),
                         CORBA::Object::_nil ());
     }
@@ -50,12 +59,24 @@ Round_Robin_Strategy::replica (CORBA::Environment &ACE_TRY_ENV)
   // next call to this method.
   this->next_replica_++;
 
+  // @@ Ossama: once we select a replica, shouldn't we temporarily
+  // change its load so it does not get swamped with all the new
+  // clients. Think about the current scenario:
+  // - Replica A has the lowest load, by far.
+  // - 5 clients connect at the same time.
+  // - If the load on A does not grow fast enough all 5 clients will
+  // end up on replica A, and then they will be kicked out and
+  // shuffled around.
+  // While if we bump the load for a few seconds then the next client
+  // will go to other place.  Or set a flag that says 'load
+  // invalidated, waiting for an update' (i like the latter better).
   return proxy_servant->replica ();
 }
 
 int
 Round_Robin_Strategy::insert (ReplicaProxy_Impl *proxy)
 {
+  // @@ Ossama: more code that is not thread safe
   int r = this->proxies_.insert (proxy);
   this->next_replica_ = this->proxies_.begin ();
   return r;
@@ -64,6 +85,7 @@ Round_Robin_Strategy::insert (ReplicaProxy_Impl *proxy)
 int
 Round_Robin_Strategy::remove (ReplicaProxy_Impl *proxy)
 {
+  // @@ Ossama: more code that is not thread safe
   int r = this->proxies_.remove (proxy);
   this->next_replica_ = this->proxies_.begin ();
   return r;
diff --git a/TAO/orbsvcs/orbsvcs/Round_Robin_Strategy.h b/TAO/orbsvcs/orbsvcs/Round_Robin_Strategy.h
index e65d8cdb39c..a279071a46f 100644
--- a/TAO/orbsvcs/orbsvcs/Round_Robin_Strategy.h
+++ b/TAO/orbsvcs/orbsvcs/Round_Robin_Strategy.h
@@ -25,6 +25,13 @@
 // Forward declarations
 class ReplicaProxy_Impl;
 
+// @@ Ossama: is this the kind of data structure that you want to use
+// in this case? I mean the iterator is invalidated on each insertion
+// and extraction, the class is not thread safe and it is hard to find
+// the element with the least load...  Then again the number of
+// servers should be small (say around 10), and additions/removals
+// rare, so the data structure is probably not a real problem.
+
 typedef ACE_Unbounded_Set<ReplicaProxy_Impl *> ReplicaProxySet;
 typedef ACE_Unbounded_Set_Iterator<ReplicaProxy_Impl *> ReplicaProxySetIterator;
 
diff --git a/TAO/orbsvcs/tests/LoadBalancing/Hash_Replica.idl b/TAO/orbsvcs/tests/LoadBalancing/Hash_Replica.idl
index 47f19b1a0ac..8ebfbfa7e8b 100644
--- a/TAO/orbsvcs/tests/LoadBalancing/Hash_Replica.idl
+++ b/TAO/orbsvcs/tests/LoadBalancing/Hash_Replica.idl
@@ -1,6 +1,8 @@
 // -*- C++ -*-
 // $Id$
 
+// @@ Ossama: we should call this class just 'Hasher' it shouldn't be
+// so obvious that the service must be designed to be load balanced.
 interface Hash_Replica
 {
   // = DESCRIPTION
diff --git a/TAO/orbsvcs/tests/LoadBalancing/Hash_ReplicaControl.cpp b/TAO/orbsvcs/tests/LoadBalancing/Hash_ReplicaControl.cpp
index 866f8d43429..74847db60c6 100644
--- a/TAO/orbsvcs/tests/LoadBalancing/Hash_ReplicaControl.cpp
+++ b/TAO/orbsvcs/tests/LoadBalancing/Hash_ReplicaControl.cpp
@@ -57,6 +57,8 @@ Hash_ReplicaControl::handle_timeout (const ACE_Time_Value &,
   this->request_count_ = 0;
   this->interval_start_ = ACE_OS::gettimeofday ();
 
+  // @@ Ossama: here is the dampening algorithm that i implemented, it
+  // is not rocket science, but helps...
   this->current_load_ =
     0.9F * this->current_load_ + 0.1F * load;
 
@@ -84,6 +86,8 @@ Hash_ReplicaControl::request_received (void)
 void
 Hash_ReplicaControl::request_rejected (CORBA::Environment &ACE_TRY_ENV)
 {
+  // @@ Ossama: notice how we reject a single request.  Maybe the
+  // advisory should include how many are we supposed to shed?
   this->replica_.reject_requests (0);
   ACE_THROW (PortableServer::ForwardRequest (
                  CORBA::Object::_duplicate (this->group_.in ())));
diff --git a/TAO/orbsvcs/tests/LoadBalancing/Hash_ReplicaControl.h b/TAO/orbsvcs/tests/LoadBalancing/Hash_ReplicaControl.h
index 264f68edace..912e5e550f6 100644
--- a/TAO/orbsvcs/tests/LoadBalancing/Hash_ReplicaControl.h
+++ b/TAO/orbsvcs/tests/LoadBalancing/Hash_ReplicaControl.h
@@ -10,6 +10,8 @@
 
 class Hash_ReplicaControl;
 
+// @@ Ossama: notice how i integrated the reactor into the control
+// loop, to periodically send the load...
 class Timeout_Adapter : public ACE_Event_Handler
 {
 public:
@@ -23,6 +25,8 @@ private:
   Hash_ReplicaControl *adaptee_;
 };
 
+// @@ Ossama: remember that we want to implement load balancing using
+// interceptors.
 class Hash_ReplicaControl : public virtual POA_LoadBalancing::ReplicaControl
 {
 public:
diff --git a/TAO/orbsvcs/tests/LoadBalancing/client.cpp b/TAO/orbsvcs/tests/LoadBalancing/client.cpp
index 8f875e0be8c..83911b71a3a 100644
--- a/TAO/orbsvcs/tests/LoadBalancing/client.cpp
+++ b/TAO/orbsvcs/tests/LoadBalancing/client.cpp
@@ -91,7 +91,7 @@ run_test (int iterations,
   ACE_Time_Value tv (0, timeout * 1000);
   ACE_Throughput_Stats stats;
   ACE_UINT64 test_start = ACE_OS::gethrtime ();
-  
+
   for (long i = 0; i != iterations; ++i)
     {
       ACE_UINT64 call_start = ACE_OS::gethrtime ();
@@ -99,7 +99,7 @@ run_test (int iterations,
       hasher->do_hash ("This is a silly test", ACE_TRY_ENV);
       ACE_CHECK;
       ACE_UINT64 end = ACE_OS::gethrtime ();
-      
+
       stats.sample (end - test_start, end - call_start);
       ACE_OS::sleep (tv);
     }