[#TODO] connection and pool improvements

[jeanouii]

No description provided.

[mattrpav]

I need to think through impacts of making PooledConnectionFactory AutoCloseable. It usually is more of an init/destroy or start/stop lifecycle since it is long lived for the lifetime of the JVM.

Adding AutoCloseable may lead to a lot of users getting WARN from IDE

[jeanouii]

When using the resource adapter inside an app server for example (Apache TomEE), the connection pool might be created and destroyed multiple times without restart the entire server (deploy / undeploy). It delegates to stop(). For tests it's convenient to avoid leaking a connection factory from one test to the other or having to cope with try/catch.

It's not the end of the world. I can revert that and rework all the tests. I don't think it harms though.

[jeanouii]

ok to merge @mattrpav

[mattrpav]

@cshannon thoughts on adding AutoCloseable to PooledConnectionFactory in a patch release? I think this will make IDEs bark at users' existing code and create false negatives.

Most things today (Camel routes, etc) use start()/stop() and not a try-with-resources.

edit: Another option would be to create a sub-class (ie AutoPooledConnectionFactory) that extends the existing class and adds only the close() method.

[cshannon]

@cshannon thoughts on adding AutoCloseable to PooledConnectionFactory in a patch release? I think this will make IDEs bark at users' existing code and create false negatives.

I guess it could cause some warnings to be thrown etc. in an IDE but it shouldn't break anyone's code or have any real other effects so I don't really think it's a big deal to add it. It also wouldn't hurt to just add it in 6.3.0 (ie not backport to a patch release). I guess it depends on how useful it is to add it for fixing tests and backports.

I don't really think we need a subclass because adding the interface on the existing class won't actually cause any issues.

[jeanouii]

It's backward compatible and won't break for sure.
On integration code (not only on tests), like in TomEE for example, having the try-with-resources is great for deploy/undeploy.
It was convenient for me to cleanup the tests, if that's something you guys are annoyed with, I'm happy to remove it and replace try-with-resources with try/catch/finally in tests.

Let me know what you prefer.

[cshannon]

Let me know what you prefer.

I would prefer to keep it as it is backwards compatible and provides a lot of benefits and there is no risk of breaking anything. Adding another subclass doesn't make sense to me as someone would now have to know to use it.

[jbonofre]

I think we could consider AutoCloseable as a kind of backward compatible: the user can use try on resource or the "old style" 😄
I'm not fan of having a subclass because there's a risk it's not actually used/visible.

[mattrpav]

Hear my out on this point though-- AutoCloseable isn't just another interface tagged to a class. Static code scanners and bug finders look for code patterns to and generate reports that drive approval for pull requests. While not as impactful as 'Serializable' it is in this category of more 'special' interfaces.

The standard/default use case for PooledConnectionFactory is to not use a try-with-resources. Feels like adding this is for the test use case vs the primary coded use case.

Feels like if we add AutoCloseable we'll make a worse Developer Experience for the standard use case.

Example:

[cshannon]

The standard/default use case for PooledConnectionFactory is to not use a try-with-resources. Feels like adding this is for the test use case vs the primary coded use case.

Thinking about it more that makes sense as it's a pooled factory so users really wouldn't ever be closing it immediately with try-with-resources. I can't really think of a use case where you'd use a pooled factory and immediately close it with try-with-resources as if you were going to do that you'd just use a normal connection.

For the analysis part I was mostly thinking of it from an IDE issue (which isn't that big of a deal but a little annoying) but the static analysis issue of builds breaking with things like spot bugs is certainly a problem and would produce annoying false negatives.

I guess based on that we should leave it off and go back to try/finally. I'm not really in favor of a subclass unless there is a real use case, or another option is we just add a subclass only in the testing package for unit tests because it doesn't seem beneficial to add autocloseable if there is no use case for a real application.

[jeanouii]

I removed the AutoClosable if that's the consensus.