[Discuss] Removal of Thread Local Connection Pooling

[Jacob Barrett]

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 

GEODE-6594 - https://issues.apache.org/jira/browse/GEODE-6594 

PR 3394 - https://github.com/apache/geode/pull/3394 

Re: [Discuss] Removal of Thread Local Connection Pooling

[Anthony Baker]

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  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 
> 
> GEODE-6594 - https://issues.apache.org/jira/browse/GEODE-6594 
> 
> PR 3394 - https://github.com/apache/geode/pull/3394 
> 
> 

Re: [DISCUSS] Move or remove org.apache.geode.admin

[Jacob Barrett]

I figured I’d take this discussion up with the experts and founding fathers. ;)

https://github.com/semver/semver/issues/508

-jake


> On Apr 4, 2019, at 8:23 AM, Anthony Baker  wrote:
> 
> Let’s separate the discussion into these parts:
> 
> - What does SemVer say and how do we apply it
> - When should we remove deprecated code
> - Should we remove the admin source code entirely
> 
> 
> 1) SemVer
> 
> Straight from the spec:
> 
>> Major version X (X.y.z | X > 0) MUST be incremented if any backwards 
>> incompatible changes are introduced to the public API
> 
> @Jens - doesn’t “new major version” imply n.0.0?
> 
> Downstream consumers of SemVer-compliant software expect that “backwards 
> compatibility” means they can upgrade to a new minor or patch release and 
> things will continue to just work—no changes required.  If you think about 
> how we upgrade our library dependencies we have that same expectation.  When 
> I upgrade from log4j 2.11.1 to log4j 2.11.2 I don’t expect to have to modify 
> any source code.  
> 
> IMO, the clarity that SemVer brings to the question of “Can I upgrade 
> safely?” Is its main benefit.  If we blur the lines and break stuff without 
> signaling via major versions, then our users will be less inclined to trust 
> us and will upgrade less frequently IMO.
> 
> 3) Deprecation
> 
> Unlike users, SemVer places no value judgement on version numbers and assumes 
> that major version numbers are “free” (aka infinite).  Some projects have 
> major versions like 42.0.0 (wow!).  Unfortunately most users *do* expect 
> certain things like new features from major versions.  And if the cost of the 
> upgrade is higher than the value gained from the upgrade then there’s a 
> strong disincentive to move forward.  We’ve all seen OSS communities that 
> have fragmented due to the cost of changing to a new major version (e.g. 
> python).  I’d sure like to avoid that.
> 
> Clearly we have a lot of long-deprecated API’s that we want to clean up.  
> That’s important to *us*.  I also want to understand why our *users* will 
> want to upgrade to a geode-2.0.0 release.  Let’s spin off a separate thread 
> to discuss what that looks like and how we handle the transition using 
> branches, prereleases, or other mechanisms to manage breaking changes.
> 
> 2) Admin source code
> 
> This is a grey area for me.  While technically it is part of the API, it’s 
> also true that it’s been broken / unusable (not to mention obsolete) for the 
> entire time it’s been part of the geode project.  +1 to remove.
> 
> 
> Anthony
> 
> 
>> On Apr 4, 2019, at 6:36 AM, Jens Deppe  wrote:
>> 
>> I'm not sure if I'm interpreting various parts of this conversation
>> correctly, but it seems that one view being assumed is that the actual
>> removal of deprecated APIs *must* happen in the first version of a major
>> release bump. i.e. for this discussion that would be *2.0.0*. I don't
>> believe this is a correct interpretation of the following; taken from
>> https://semver.org/
>> 
>> Before you completely remove the functionality *in a new major release*
>>> there should be at least one minor release that contains the deprecation so
>>> that users can smoothly transition to the new API.
>> 
>> 
>> There is no mention of it needing to happen in the *n.0.0* version; just
>> *sometime* within a new major version.
>> 
>> Given that, these APIs were definitely deprecated before 1.0 and I believe
>> we're within the definition of *semver* to be free to remove them now.
>> 
>> +1 to removing them any time before 2.0.0.
>> 
>> --Jens
>> 
>>> On Wed, Apr 3, 2019 at 7:38 PM Jacob Barrett  wrote:
>>> 
>>> That’s your interpretation of semver. I interpret The API not to be broken
>>> within a major as those that are still valid when the major is released
>>> despite the availability of deprecated symbols from previous major. Just
>>> because I can access symbols does not make it API. API is the combination
>>> of documentation, annotation and availability of the symbols. If the
>>> symbols are available but marked deprecated and not documented anymore then
>>> it is. It API.
>>> 
>>> Yes under your interpretation it would require that availability be
>>> removed at the major release too so that a consumer that ignore deprecation
>>> warnings when compiling with the new version got a compilation or runtime
>>> error later up updating a minor.
>>> 
>>> I think that error is well deserved. We do a disservice to our customers
>>> by allowing them to continue to limp along on deprecated, unmaintained and
>>> untested APIs because we missed an arbitrary window to remove the symbols.
>>> 
>>> If however the community agrees with you interpretation then we must make
>>> sure a ticket is created for the next major with a list of deprecated items
>>> and updated when new items are deprecated so that the task is performed at
>>> the major release. A chore needs to be undertaken to remove all deprecated
>>> APIs at the start of each major dev

Re: [Geode Build] Are the idea and eclipse plugins being used by anyone?

[Robert Houghton]

@upthewaterspout The plugin generates .iml, .ipr files used by old versions
of IntelliJ. The plugin does not give a way to create or modify the
currently-used XML files that live inside of the /.idea directory.

+1 to removal of the idea plugin.

On Wed, Apr 3, 2019 at 3:22 PM Dan Smith  wrote:

> I was under the impression that intellij's import actually used some of the
> information in the idea{} blocks in the build. See
> https://docs.gradle.org/current/userguide/idea_plugin.html.
>
> But if the import works well without that extra configuration, I think we
> should get rid of it.
>
> -Dan
>
> On Wed, Apr 3, 2019 at 2:32 PM Michael Oleske  wrote:
>
> > I tend to import into Intellij the way you have described.  I've never
> > used ./gradlew
> > idea to configure.
> >
> > -michael
> >
> > On Wed, Apr 3, 2019 at 2:24 PM Patrick Rhomberg 
> > wrote:
> >
> > > I'm eyeballing the ide.gradle file we have and am wondering if it is
> > cruft
> > > from many iterations of both Gradle and IntelliJ/Eclipse ago.
> > >
> > > In my own IntelliJ workflow, I have always just asked IntelliJ to open
> > the
> > > gradle project via the build.gradle itself, and it's easy to import
> other
> > > modules into my workspace via Import Module from Existing Sources...
> > I've
> > > never had to deal with running ./gradlew idea to configure a project
> IML
> > > which I needed to merge against another project file.
> > >
> > > Is this the workflow others are using?  Or is the idea plugin here a
> > > hanger-on that should be removed as no longer necessary?
> > >
> > > Also, as an IntelliJ user, I have zero experience on how the workflow
> for
> > > Eclipse is similar or different.  The closures in our ide.gradle appear
> > to
> > > be similar, but I don't have the Eclipse-side context to confirm or
> deny
> > > that.
> > >
> > > Thanks in advance for any additional insight.
> > >
> > > Imagination is Change.
> > > ~Patrick
> > >
> >
>

Re: [Geode Build] Are the idea and eclipse plugins being used by anyone?

[Dan Smith]

+1 to removing the idea plugin and the eclipse plugin as well. There is an
eclipse plugin  that
lets you import gradle projects similar to the intellij one. I think that
is what our eclipse developers are using now.

I still think the intellij import interacts with the idea plugin in some
way, but maybe we no longer need to configure the import. This is what the
gradle docs say about the intellij import:

If you simply want to load a Gradle project into IntelliJ IDEA, then use
the IDE’s import facility
.
You do not need to apply this plugin to import your project into IDEA,
although if you do,
the import will take account of any extra IDEA configuration you have that
doesn’t directly modify
the generated files — see the Configuration

section for more details.


-Dan

On Fri, Apr 5, 2019 at 9:23 AM Robert Houghton  wrote:

> @upthewaterspout The plugin generates .iml, .ipr files used by old versions
> of IntelliJ. The plugin does not give a way to create or modify the
> currently-used XML files that live inside of the /.idea directory.
>
> +1 to removal of the idea plugin.
>
> On Wed, Apr 3, 2019 at 3:22 PM Dan Smith  wrote:
>
> > I was under the impression that intellij's import actually used some of
> the
> > information in the idea{} blocks in the build. See
> > https://docs.gradle.org/current/userguide/idea_plugin.html.
> >
> > But if the import works well without that extra configuration, I think we
> > should get rid of it.
> >
> > -Dan
> >
> > On Wed, Apr 3, 2019 at 2:32 PM Michael Oleske 
> wrote:
> >
> > > I tend to import into Intellij the way you have described.  I've never
> > > used ./gradlew
> > > idea to configure.
> > >
> > > -michael
> > >
> > > On Wed, Apr 3, 2019 at 2:24 PM Patrick Rhomberg 
> > > wrote:
> > >
> > > > I'm eyeballing the ide.gradle file we have and am wondering if it is
> > > cruft
> > > > from many iterations of both Gradle and IntelliJ/Eclipse ago.
> > > >
> > > > In my own IntelliJ workflow, I have always just asked IntelliJ to
> open
> > > the
> > > > gradle project via the build.gradle itself, and it's easy to import
> > other
> > > > modules into my workspace via Import Module from Existing Sources...
> > > I've
> > > > never had to deal with running ./gradlew idea to configure a project
> > IML
> > > > which I needed to merge against another project file.
> > > >
> > > > Is this the workflow others are using?  Or is the idea plugin here a
> > > > hanger-on that should be removed as no longer necessary?
> > > >
> > > > Also, as an IntelliJ user, I have zero experience on how the workflow
> > for
> > > > Eclipse is similar or different.  The closures in our ide.gradle
> appear
> > > to
> > > > be similar, but I don't have the Eclipse-side context to confirm or
> > deny
> > > > that.
> > > >
> > > > Thanks in advance for any additional insight.
> > > >
> > > > Imagination is Change.
> > > > ~Patrick
> > > >
> > >
> >
>

Re: [Discuss] Removal of Thread Local Connection Pooling

[John Blum]

Well articulated and a wise decision; Jake. +1

On Fri, Apr 5, 2019 at 8:24 AM Anthony Baker  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  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]

> On Apr 5, 2019, at 8:23 AM, Anthony Baker  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] Move or remove org.apache.geode.admin

[Kirk Lund]

I would be very upset and surprised as a user or developer if Geode never
has any major releases.

Using semver as the main argument to not remove a feature that has been
deprecated for 7 years is silly.

Let’s have a major release! 2.0 by summer.

On Fri, Apr 5, 2019 at 8:38 AM Jacob Barrett  wrote:

> I figured I’d take this discussion up with the experts and founding
> fathers. ;)
>
> https://github.com/semver/semver/issues/508
>
> -jake
>
>
> > On Apr 4, 2019, at 8:23 AM, Anthony Baker  wrote:
> >
> > Let’s separate the discussion into these parts:
> >
> > - What does SemVer say and how do we apply it
> > - When should we remove deprecated code
> > - Should we remove the admin source code entirely
> >
> >
> > 1) SemVer
> >
> > Straight from the spec:
> >
> >> Major version X (X.y.z | X > 0) MUST be incremented if any backwards
> incompatible changes are introduced to the public API
> >
> > @Jens - doesn’t “new major version” imply n.0.0?
> >
> > Downstream consumers of SemVer-compliant software expect that “backwards
> compatibility” means they can upgrade to a new minor or patch release and
> things will continue to just work—no changes required.  If you think about
> how we upgrade our library dependencies we have that same expectation.
> When I upgrade from log4j 2.11.1 to log4j 2.11.2 I don’t expect to have to
> modify any source code.
> >
> > IMO, the clarity that SemVer brings to the question of “Can I upgrade
> safely?” Is its main benefit.  If we blur the lines and break stuff without
> signaling via major versions, then our users will be less inclined to trust
> us and will upgrade less frequently IMO.
> >
> > 3) Deprecation
> >
> > Unlike users, SemVer places no value judgement on version numbers and
> assumes that major version numbers are “free” (aka infinite).  Some
> projects have major versions like 42.0.0 (wow!).  Unfortunately most users
> *do* expect certain things like new features from major versions.  And if
> the cost of the upgrade is higher than the value gained from the upgrade
> then there’s a strong disincentive to move forward.  We’ve all seen OSS
> communities that have fragmented due to the cost of changing to a new major
> version (e.g. python).  I’d sure like to avoid that.
> >
> > Clearly we have a lot of long-deprecated API’s that we want to clean
> up.  That’s important to *us*.  I also want to understand why our *users*
> will want to upgrade to a geode-2.0.0 release.  Let’s spin off a separate
> thread to discuss what that looks like and how we handle the transition
> using branches, prereleases, or other mechanisms to manage breaking changes.
> >
> > 2) Admin source code
> >
> > This is a grey area for me.  While technically it is part of the API,
> it’s also true that it’s been broken / unusable (not to mention obsolete)
> for the entire time it’s been part of the geode project.  +1 to remove.
> >
> >
> > Anthony
> >
> >
> >> On Apr 4, 2019, at 6:36 AM, Jens Deppe  wrote:
> >>
> >> I'm not sure if I'm interpreting various parts of this conversation
> >> correctly, but it seems that one view being assumed is that the actual
> >> removal of deprecated APIs *must* happen in the first version of a major
> >> release bump. i.e. for this discussion that would be *2.0.0*. I don't
> >> believe this is a correct interpretation of the following; taken from
> >> https://semver.org/
> >>
> >> Before you completely remove the functionality *in a new major release*
> >>> there should be at least one minor release that contains the
> deprecation so
> >>> that users can smoothly transition to the new API.
> >>
> >>
> >> There is no mention of it needing to happen in the *n.0.0* version; just
> >> *sometime* within a new major version.
> >>
> >> Given that, these APIs were definitely deprecated before 1.0 and I
> believe
> >> we're within the definition of *semver* to be free to remove them now.
> >>
> >> +1 to removing them any time before 2.0.0.
> >>
> >> --Jens
> >>
> >>> On Wed, Apr 3, 2019 at 7:38 PM Jacob Barrett 
> wrote:
> >>>
> >>> That’s your interpretation of semver. I interpret The API not to be
> broken
> >>> within a major as those that are still valid when the major is released
> >>> despite the availability of deprecated symbols from previous major.
> Just
> >>> because I can access symbols does not make it API. API is the
> combination
> >>> of documentation, annotation and availability of the symbols. If the
> >>> symbols are available but marked deprecated and not documented anymore
> then
> >>> it is. It API.
> >>>
> >>> Yes under your interpretation it would require that availability be
> >>> removed at the major release too so that a consumer that ignore
> deprecation
> >>> warnings when compiling with the new version got a compilation or
> runtime
> >>> error later up updating a minor.
> >>>
> >>> I think that error is well deserved. We do a disservice to our
> customers
> >>> by allowing them to continue to limp along on deprecated, unmaintained
> and
> >>>