comments

2 files changed, 58 insertions, 21 deletions

diff --git a/cpp/src/Ice/LocatorInfo.cpp b/cpp/src/Ice/LocatorInfo.cpp
index ca6ab4c0fa6..8d24f78e938 100644
--- a/cpp/src/Ice/LocatorInfo.cpp
+++ b/cpp/src/Ice/LocatorInfo.cpp
@@ -53,7 +53,7 @@ IceInternal::LocatorManager::destroy()
 LocatorInfoPtr
 IceInternal::LocatorManager::get(const LocatorPrx& locator)
 {
-    if (!locator)
+    if(!locator)
     {
 	return 0;
     }
@@ -66,20 +66,20 @@ IceInternal::LocatorManager::get(const LocatorPrx& locator)
 
     map<LocatorPrx, LocatorInfoPtr>::iterator p = _table.end();
     
-    if (_tableHint != _table.end())
+    if(_tableHint != _table.end())
     {
-	if (_tableHint->first == locator)
+	if(_tableHint->first == locator)
 	{
 	    p = _tableHint;
 	}
     }
     
-    if (p == _table.end())
+    if(p == _table.end())
     {
 	p = _table.find(locator);
     }
 
-    if (p == _table.end())
+    if(p == _table.end())
     {
 	//
 	// Rely on locator identity for the adapter table. We want to
@@ -114,7 +114,7 @@ IceInternal::LocatorAdapterTable::get(const string& adapter, ::std::vector<Endpo
     
     std::map<std::string, std::vector<EndpointPtr> >::const_iterator p = _adapterEndpointsMap.find(adapter);
     
-    if (p != _adapterEndpointsMap.end())
+    if(p != _adapterEndpointsMap.end())
     {
 	endpoints = p->second;
 	return true;
@@ -169,6 +169,10 @@ IceInternal::LocatorInfo::operator<(const LocatorInfo& rhs) const
     return _locator < rhs._locator;
 }
 
+// TODO: ML: Should be const. I know that I didn't do this correctly
+// for other classes (such as RouterInfo) as well. However, we need to
+// start doing this right :-) Can you please fix const-correctness for the
+// locator and router stuff?
 LocatorPrx
 IceInternal::LocatorInfo::getLocator()
 {
@@ -183,7 +187,7 @@ IceInternal::LocatorInfo::getLocatorRegistry()
 {
     IceUtil::Mutex::Lock sync(*this);
     
-    if (!_locatorRegistry) // Lazy initialization.
+    if(!_locatorRegistry) // Lazy initialization.
     {
 	_locatorRegistry = _locator->getRegistry();
     }
@@ -207,7 +211,7 @@ IceInternal::LocatorInfo::getEndpoints(const ReferencePtr& ref, ::std::vector<En
 	    try
 	    {
 		ObjectPrx object = _locator->findAdapterByName(ref->adapterId);
-		if (object)
+		if(object)
 		{
 		    endpoints = object->__reference()->endpoints;
 		}
@@ -225,20 +229,34 @@ IceInternal::LocatorInfo::getEndpoints(const ReferencePtr& ref, ::std::vector<En
 	    cached = true;	
 	}
 
+	// TODO: ML: Don't use size to check for emptyness. Use !endpoints.empty().
 	if(endpoints.size() > 0)
 	{
-	    if (ref->instance->traceLevels()->location >= 1)
+	    if(ref->instance->traceLevels()->location >= 1)
 	    {
+		// TODO: ML: "object"? You mean "proxy"! Or even
+		// better, since only the endpoints count here, use
+		// "endpoints" instead of "object".
+
 		Trace out(ref->instance->logger(), ref->instance->traceLevels()->locationCat);
 		if(cached)
+		{
+		    // TODO: ML: "local locator table"? Isn't this always
+		    // local? I suggest to remove the "local".
 		    out << "found object in local locator table\n";
+		}
 		else
+		{
+		    // TODO: ML: "found"? Shouldn't this read
+		    // "retrieved endpoints from locator".
 		    out << "found object in locator\n";
+		}
+		// TODO: ML: Why do you log the identity? What meaning does it have?
 		out << "identity = " << identityToString(ref->identity) << "\n";
 		out << "adapter = " << ref->adapterId << "\n";
 		const char* sep = endpoints.size() > 1 ? ":" : "";
 		ostringstream o;
-		transform(endpoints.begin(), endpoints.end(), ostream_iterator<string>(o,sep),
+		transform(endpoints.begin(), endpoints.end(), ostream_iterator<string>(o, sep),
 			  ::Ice::constMemFun(&Endpoint::toString));
 		out << "endpoints = " << o.str();
 	    }
@@ -248,36 +266,46 @@ IceInternal::LocatorInfo::getEndpoints(const ReferencePtr& ref, ::std::vector<En
     return cached;
 }
 
+// TODO: ML: Why a separate operation? Why not add this stuff to
+// getEndpoings() above? What's the point of getting endpoints without
+// caching them?
 void 
 IceInternal::LocatorInfo::addEndpoints(const ReferencePtr& ref, const ::std::vector<EndpointPtr>& endpoints)
 {
     assert(!endpoints.empty());
 
-    if (!ref->adapterId.empty())
+    if(!ref->adapterId.empty())
     {
 	_adapterTable->add(ref->adapterId, endpoints);
 
-	if (ref->instance->traceLevels()->location >= 2)
+	if(ref->instance->traceLevels()->location >= 2)
 	{
 	    Trace out(ref->instance->logger(), ref->instance->traceLevels()->locationCat);
+	    // TODO: Since this function should be removed, no
+	    // separate log message is necessary either. Instead use
+	    // "retrieved endpoints from locator, adding to locator
+	    // table" above.
 	    out << "added adapter to local locator table\n";
 	    out << "adapter = "  << ref->adapterId;
 	}
     }
 }
 
+// TODO: ML: Should be named to "clearCache".
 void 
 IceInternal::LocatorInfo::removeEndpoints(const ReferencePtr& ref)
 {
-    if (!ref->adapterId.empty())
+    if(!ref->adapterId.empty())
     {
 	_adapterTable->remove(ref->adapterId);
 
-	if (ref->instance->traceLevels()->location >= 2)
+	if(ref->instance->traceLevels()->location >= 2)
 	{
 	    Trace out(ref->instance->logger(), ref->instance->traceLevels()->locationCat);
+	    // TODO: ML: "removed endpoints from locator table"
 	    out << "removed adapter from local locator table\n";
 	    out << "adapter = "  << ref->adapterId;
+	    // TODO: ML: Log endpoints, like in getEndpoints().
 	}    
     }
 }
diff --git a/cpp/src/Ice/Proxy.cpp b/cpp/src/Ice/Proxy.cpp
index 0ffd718da5f..b99a8f0c61e 100644
--- a/cpp/src/Ice/Proxy.cpp
+++ b/cpp/src/Ice/Proxy.cpp
@@ -545,6 +545,10 @@ IceProxy::Ice::Object::__handleException(const LocalException& ex, int& cnt)
 	// We always retry on a close connection exception, as this
 	// indicates graceful server shutdown.
 	//
+	// TODO: ML: Perhaps we should have a limit on this too?
+	// Otherwise a rogue server could let the client retry
+	// forever.
+	//
     }
     catch(const SocketException&)
     {
@@ -563,7 +567,8 @@ IceProxy::Ice::Object::__handleException(const LocalException& ex, int& cnt)
     LoggerPtr logger = _reference->instance->logger();
     
     const std::vector<int>& retryIntervals = _reference->instance->proxyFactory()->getRetryIntervals();
-    
+
+    // TODO: ML: Loose C-style cast.
     if(cnt > (int)retryIntervals.size())
     {
 	if(traceLevels->retry >= 1)
@@ -580,7 +585,7 @@ IceProxy::Ice::Object::__handleException(const LocalException& ex, int& cnt)
 	out << "re-trying operation call";
 	if(cnt > 0 && retryIntervals[cnt - 1] > 0)
 	{
-	    out << " in " << retryIntervals[cnt - 1] << " (ms)";
+	    out << " in " << retryIntervals[cnt - 1] << "ms";
 	}
 	out << " because of exception\n" << ex;
     }
@@ -589,7 +594,7 @@ IceProxy::Ice::Object::__handleException(const LocalException& ex, int& cnt)
     {
 	//
 	// Sleep before retrying. TODO: is it safe to sleep here
-	// with the mutex locked?
+	// with the mutex locked? TODO: ML: Why not sleep outside the mutex lock?</ml>
 	//
 	IceUtil::ThreadControl::sleep(IceUtil::Time::milliSeconds(retryIntervals[cnt - 1]));
     }
@@ -887,10 +892,14 @@ IceDelegateM::Ice::Object::setup(const ReferencePtr& ref)
 	    }
 	    else if(__reference->locatorInfo)
 	    {
+		//
+		// TODO: ML: This call is confusing. It should be:
+		// endpoints = __reference->locatorInfo->getEndpoints(__reference, cached);
+		// Please change the signature accordingly.
+		//
 		cached = __reference->locatorInfo->getEndpoints(__reference, endpoints);
 	    }
 
-	    
 	    vector<EndpointPtr> filteredEndpoints = filterEndpoints(endpoints);
 	    if(filteredEndpoints.empty())
 	    {
@@ -911,11 +920,11 @@ IceDelegateM::Ice::Object::setup(const ReferencePtr& ref)
 		    TraceLevelsPtr traceLevels = __reference->instance->traceLevels();
 		    LoggerPtr logger = __reference->instance->logger();
 		    
-		    if(traceLevels->retry >= 1)
+		    if(traceLevels->retry >= 2)
 		    {
 			Trace out(logger, traceLevels->retryCat);
-			out << "connection to cached endpoint failed, removing endpoint from cache\n"
-			    << "and trying one more time\n" << ex;
+			out << "connection to cached endpoints failed\n"
+			    << "removing endpoints from cache and trying one more time\n" << ex;
 		    }
 		    
 		    assert(__reference->locatorInfo);