[Discuss] Removal of Thread Local Connection Pooling

[Jacob Barrett <jb...@pivotal.io>]

Devs,

The current connection pooling implementation contains a setting that enables a secondary pool that is thread local. See ClientCacheFactory. setPoolThreadLocalConnections method for details. This thread local pooling was added to reduce contention on the primary connection pool under high thread count or load. As of the completion of GEODE-6515 there is no longer a contention issue in the primary pool under high thread count or load, effectively rendering thread local pooling a no-op. Thread local pooling also introduces many complexities into the primary pool management of connections since a connection can either be in the primary pool or a pool for each thread. Connection idle timeout, lifetime expiration and load conditioning all have to work around thread local connections making for some interesting relationships and behavior. Additionally clients of thread local connections needed to call Pool.releaseThreadLocalConnections prior to the thread terminating or connections would be held until the GC finalized the thread local storage. Use of thread local connections typically mean significantly high connection counts on the servers since each thread could horde connections to each server regardless of current workload need.

I propose that we deprecate all the public APIs for thread local connections and immediately remove the behavior. The API methods will go from an effective no-op to an actual no-op. A warning will be logged when ClientCacheFactory. setPoolThreadLocalConnections is used. Internally all thread local connection implementation will be removed. The net effect to the client will be the same as promised with thread local connections, that content on the primary pool is reduced for high thread count and load. Additionally, the connection count to each server will be reduced since threads won’t be holding connections to servers they are not actively communicating with. All in all this will be a net-positive improvement for the consuming client and the distributed system as a whole. Internally this will be huge win as it removes complexity and opens a path to removing even more complexity in idle connection timeouts, connection lifetime expiration, and load conditioning. Please see GEODE-6594 for more details.

I realize this is sort of a grey area. An obvious questions is, “doesn’t this violate semver by removing a feature in a minor?” My answer is a solid, “nope!” The feature is defined as “reducing contention on the connection pool”, which is provided now by default in the refactored connection pool. All that is being done is deprecating and warning that a useless API is being removed in the next major release. The feature remains intact and client code does not need to be re-compiled or changed. 

I would love to hear if anyone has any concerns, questions or dissenting opinions about this proposal. PR 3394 has been opened for review of the code changes related to the removal. Please provide feedback in the PR relating only to the code and discussions about the merits of he proposal here in this email thread.

Thanks,
Jake

GEODE-6515 - https://issues.apache.org/jira/browse/GEODE-6515 <https://issues.apache.org/jira/browse/GEODE-6515>
GEODE-6594 - https://issues.apache.org/jira/browse/GEODE-6594 <https://issues.apache.org/jira/browse/GEODE-6594>
PR 3394 - https://github.com/apache/geode/pull/3394 <https://github.com/apache/geode/pull/3394>

Re: [Discuss] Removal of Thread Local Connection Pooling

[John Blum <jb...@pivotal.io>]

Well articulated and a wise decision; Jake. +1

On Fri, Apr 5, 2019 at 8:24 AM Anthony Baker <ab...@pivotal.io> wrote:

> One question:  if I’m using thread-local connections ho does that affect
> pool sizing?  Are thread-local connections included in the overall pool
> size or accounted for separately?
>
> We may want some explicit release notes if a user would need to resize
> their pools during an upgrade.
>
> Anthony
>
>
> > On Apr 5, 2019, at 6:34 AM, Jacob Barrett <jb...@pivotal.io> wrote:
> >
> > Devs,
> >
> > The current connection pooling implementation contains a setting that
> enables a secondary pool that is thread local. See ClientCacheFactory.
> setPoolThreadLocalConnections method for details. This thread local pooling
> was added to reduce contention on the primary connection pool under high
> thread count or load. As of the completion of GEODE-6515 there is no longer
> a contention issue in the primary pool under high thread count or load,
> effectively rendering thread local pooling a no-op. Thread local pooling
> also introduces many complexities into the primary pool management of
> connections since a connection can either be in the primary pool or a pool
> for each thread. Connection idle timeout, lifetime expiration and load
> conditioning all have to work around thread local connections making for
> some interesting relationships and behavior. Additionally clients of thread
> local connections needed to call Pool.releaseThreadLocalConnections prior
> to the thread terminating or connections would be held until the GC
> finalized the thread local storage. Use of thread local connections
> typically mean significantly high connection counts on the servers since
> each thread could horde connections to each server regardless of current
> workload need.
> >
> > I propose that we deprecate all the public APIs for thread local
> connections and immediately remove the behavior. The API methods will go
> from an effective no-op to an actual no-op. A warning will be logged when
> ClientCacheFactory. setPoolThreadLocalConnections is used. Internally all
> thread local connection implementation will be removed. The net effect to
> the client will be the same as promised with thread local connections, that
> content on the primary pool is reduced for high thread count and load.
> Additionally, the connection count to each server will be reduced since
> threads won’t be holding connections to servers they are not actively
> communicating with. All in all this will be a net-positive improvement for
> the consuming client and the distributed system as a whole. Internally this
> will be huge win as it removes complexity and opens a path to removing even
> more complexity in idle connection timeouts, connection lifetime
> expiration, and load conditioning. Please see GEODE-6594 for more details.
> >
> > I realize this is sort of a grey area. An obvious questions is, “doesn’t
> this violate semver by removing a feature in a minor?” My answer is a
> solid, “nope!” The feature is defined as “reducing contention on the
> connection pool”, which is provided now by default in the refactored
> connection pool. All that is being done is deprecating and warning that a
> useless API is being removed in the next major release. The feature remains
> intact and client code does not need to be re-compiled or changed.
> >
> > I would love to hear if anyone has any concerns, questions or dissenting
> opinions about this proposal. PR 3394 has been opened for review of the
> code changes related to the removal. Please provide feedback in the PR
> relating only to the code and discussions about the merits of he proposal
> here in this email thread.
> >
> > Thanks,
> > Jake
> >
> > GEODE-6515 - https://issues.apache.org/jira/browse/GEODE-6515 <
> https://issues.apache.org/jira/browse/GEODE-6515>
> > GEODE-6594 - https://issues.apache.org/jira/browse/GEODE-6594 <
> https://issues.apache.org/jira/browse/GEODE-6594>
> > PR 3394 - https://github.com/apache/geode/pull/3394 <
> https://github.com/apache/geode/pull/3394>
> >
>
>

-- 
-John
john.blum10101 (skype)

Re: [Discuss] Removal of Thread Local Connection Pooling

[Jacob Barrett <jb...@pivotal.io>]

> On Apr 5, 2019, at 8:23 AM, Anthony Baker <ab...@pivotal.io> wrote:
> 
> One question:  if I’m using thread-local connections ho does that affect pool sizing?  Are thread-local connections included in the overall pool size or accounted for separately?

On the client side thread local pool just pulls from the primary pool so any limit on the primary pool applies to the thread local. By default the client pool is unbounded so it isn’t a problem. If you did bound it to like 100 connections and had 200 threads then 100 of those threads will fail to get connections.

It really effects the server more. Assuming you have 4 servers with normally distributed partitioned data, then a 1000 thread client doing single hop operations would have 4000 connections, 1 to each server for each thread. The other issues is that 3/4 of those connections are not doing anything at any given time. Given the default limit of 800 connections on the server you can see how quickly thread local connections can put you over that limit. The fixes to the primary pool contention allow us to do 1000 threads (or more) into the primary pool. So with this same client, doing normally distributed single hop operations, you could expect about 1000 connections, about 250 connections to each server. The win is that all these connections are hot, not idle. 

> We may want some explicit release notes if a user would need to resize their pools during an upgrade.

We could provide some documentation to suggest they may be able to scale down their max limits on the servers.


-Jake

Re: [Discuss] Removal of Thread Local Connection Pooling

[Anthony Baker <ab...@pivotal.io>]

One question:  if I’m using thread-local connections ho does that affect pool sizing?  Are thread-local connections included in the overall pool size or accounted for separately?

We may want some explicit release notes if a user would need to resize their pools during an upgrade.

Anthony


> On Apr 5, 2019, at 6:34 AM, Jacob Barrett <jb...@pivotal.io> wrote:
> 
> Devs,
> 
> The current connection pooling implementation contains a setting that enables a secondary pool that is thread local. See ClientCacheFactory. setPoolThreadLocalConnections method for details. This thread local pooling was added to reduce contention on the primary connection pool under high thread count or load. As of the completion of GEODE-6515 there is no longer a contention issue in the primary pool under high thread count or load, effectively rendering thread local pooling a no-op. Thread local pooling also introduces many complexities into the primary pool management of connections since a connection can either be in the primary pool or a pool for each thread. Connection idle timeout, lifetime expiration and load conditioning all have to work around thread local connections making for some interesting relationships and behavior. Additionally clients of thread local connections needed to call Pool.releaseThreadLocalConnections prior to the thread terminating or connections would be held until the GC finalized the thread local storage. Use of thread local connections typically mean significantly high connection counts on the servers since each thread could horde connections to each server regardless of current workload need.
> 
> I propose that we deprecate all the public APIs for thread local connections and immediately remove the behavior. The API methods will go from an effective no-op to an actual no-op. A warning will be logged when ClientCacheFactory. setPoolThreadLocalConnections is used. Internally all thread local connection implementation will be removed. The net effect to the client will be the same as promised with thread local connections, that content on the primary pool is reduced for high thread count and load. Additionally, the connection count to each server will be reduced since threads won’t be holding connections to servers they are not actively communicating with. All in all this will be a net-positive improvement for the consuming client and the distributed system as a whole. Internally this will be huge win as it removes complexity and opens a path to removing even more complexity in idle connection timeouts, connection lifetime expiration, and load conditioning. Please see GEODE-6594 for more details.
> 
> I realize this is sort of a grey area. An obvious questions is, “doesn’t this violate semver by removing a feature in a minor?” My answer is a solid, “nope!” The feature is defined as “reducing contention on the connection pool”, which is provided now by default in the refactored connection pool. All that is being done is deprecating and warning that a useless API is being removed in the next major release. The feature remains intact and client code does not need to be re-compiled or changed. 
> 
> I would love to hear if anyone has any concerns, questions or dissenting opinions about this proposal. PR 3394 has been opened for review of the code changes related to the removal. Please provide feedback in the PR relating only to the code and discussions about the merits of he proposal here in this email thread.
> 
> Thanks,
> Jake
> 
> GEODE-6515 - https://issues.apache.org/jira/browse/GEODE-6515 <https://issues.apache.org/jira/browse/GEODE-6515>
> GEODE-6594 - https://issues.apache.org/jira/browse/GEODE-6594 <https://issues.apache.org/jira/browse/GEODE-6594>
> PR 3394 - https://github.com/apache/geode/pull/3394 <https://github.com/apache/geode/pull/3394>
>