[GitHub] [geode] pivotal-jbarrett opened pull request #3394: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [ ] Is your initial contribution a single, squashed commit?

- [ ] Does `gradlew build` run cleanly?

- [ ] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Same as above.

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] metatype commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["metatype (GitHub)" <gi...@apache.org>]

Same.  Already deprecated on the interface API?

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

This one is totally doable since it doesn’t log.

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

I left it in there so that the warning would get logged of the xml contained this attribute.

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] metatype commented on issue #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["metatype (GitHub)" <gi...@apache.org>]

I think the documentation page on this needs to be updated as well.  See https://geode.apache.org/docs/guide/18/topologies_and_comm/cs_configuration/client_server_whats_next.html.

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

Is this method still called? I think it should be. The old test method that did it was "test010LifetimeExpireOnPoolCnx" but that was deleted. Seems like you could just make this on public, rename it, and add the Test annotation

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] metatype commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["metatype (GitHub)" <gi...@apache.org>]

Does this matter if we've already deprecated the public API?

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on issue #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

@davebarnes97 Can you please give the docs changes in https://github.com/apache/geode/pull/3394/commits/14fcf3d5ec303482018e0e070ff36be4b7f674e5.

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Check out this change now, like?

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

Good reason

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

There was something internal going on when I first got to this method. I’ll double check and see if it can go now. 

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

Since this is part of the public API it should be deprecated

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Hmmmmm interesting idea. Because we are having this message log a warning it would require that the interface gain a logger. Not sure I’d like that. I guess the method could new a logger itself. Think it’s worth it?

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on issue #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

@upthewaterspout That is a very good question. Is it better the it reflect the intent or reality. I think I would lean towards your suggestion of keeping track of the value but just ignoring it. This way the caller has its reality and we have ours. I will whip up a change. Thanks!

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

It’s package level protected. I think it can be removed though. I can’t recall why I had to leave it. I’ll check again.

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Kinda, if the interfaces is deprecated but the implementation is not then internally where we use the implementation the compiler and IJ don’t warn about the use. Adding to here too makes warnings propagate to the implementation as well. Silly.

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] metatype commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["metatype (GitHub)" <gi...@apache.org>]

Since this is an internal class can we just remove the method?

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] metatype commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["metatype (GitHub)" <gi...@apache.org>]

Should this be removed or deprecated?

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

you could also mark this one as "default" and have it do nothing as the default impl

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Solved this with @dschneider-pivotal suggestion to use default.

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

Why don't you also remove the code in CacheXmlParser that now calls the deprecated PoolFactory.setThreadLocalConnections? I know you don't need to but it seems cleaner and the product will be that much more ready to have these deprecated apis removed. It would also allow you to delete: CacheXml.THREAD_LOCAL_CONNECTIONS

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Looks like I just had it confused with the same method on Pool. Thanks!

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

something I think you could do here, that would allow you to delete all the impls of this method, is mark it here as "default" and just have it return false here as the default implementation.

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Sorry again, I'm tired, yes It is part of the API, PoolFactory is an Interface. Fixing now.


[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Sorry, misread what y'all were saying. I removed it because it isn't used internally anymore and it was package private, which isn't public API.

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

No. You are right that it could just create the logger but I think it would be better to leave like you have it now. 

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #3394: GEODE-6595: Deprecates and ignore thread local connection pool attribute.

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Good catch. I think that is exactly what I intended to do and missed it. 

[ Full content available at: https://github.com/apache/geode/pull/3394 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org