Details
-
Improvement
-
Status: Closed
-
Major
-
Resolution: Fixed
-
None
Description
I was wondering why ConnectionManagerImpl.borrowConnection that targets a particular server will not wait for a connection to be available. This is the borrow used by pr single hop. It seems like if all the cnxs to that server are busy and we are at max that it should wait. But instead it does a single scan of the pool and if it does not find an idle connection it either throws AllConnectionsInUse or it creates one. The caller sets the onlyUseExistingCnx parameter based on if the pool is at max cnxs or not (seems like this should have been done in borrowConnection instead of the caller). If it throws AllConnectionsInUse the caller catches it and ignores it and then falls into doing a non-targeted borrow that will wait for a connection but to ANY server. So our single hop optimization goes away on a busy client. Since it can’t immediately get a connection to server A, it says just give me a connection to any server sends the op to that other server and that server ends up having to forward it to server A. You would only see this problem if you set a max on your client connection pool. The default of -1 just says to always create another connection. The only workaround I know of is to not limit how many connections your client can create but that could cause other problem (i.e. too many file descriptors).
The questionable code is in the following classes:
org.apache.geode.cache.client.internal.GetOp
org.apache.geode.cache.client.internal.PutOp
org.apache.geode.cache.client.internal.DestroyOp
The all have something like this:
boolean onlyUseExistingCnx = ((poolImpl.getMaxConnections() != -1 && poolImpl.getConnectionCount() >= poolImpl.getMaxConnections()) ? true : false); op.setAllowDuplicateMetadataRefresh(!onlyUseExistingCnx); return pool.executeOn(new ServerLocation(server.getHostName(), server.getPort()), op, true, onlyUseExistingCnx);
executeOn eventually calls ConnectionManagerImpl.borrowConnection
It is unclear to me why they need allow duplicate metadata refresh if they permit borrowConnection to create a new connection to the specified server. They all catch and ignore
AllConnectionsInUseException falling through and doing non-targetted pool.execute.
The concern might have been that the pool would be full of connections to other servers preventing us from connecting to the server we need to communicate with. If we have existing connections to the server we could wait for one of those. If we don't and we are at max then we should consider closing a connection to some other server and create a connection to the server we know we need for this operation. As the server count goes up and the max connection count on the pool goes down I'm sure we are probably better off not even trying to do single hop since we might not even be able to have a connection to each server in the pool. But if we have a reasonable max connection count then it seems like we could make more of an effort to make sure a per server connection count is balanced and then wait for one of those connection to be available.
