Renew connection in PostgresChannelMessageTableSubscriber only when invalid.

[joshiste]

No description provided.

[artembilan]

I see that this is an evolution of your previous change for the #9061 .

Does this conn.isValid(1) still report OK in case of DB failover ?

Thanks

[artembilan]

I'm not very happy when we create unmanaged threads, even in tests.
Isn't there other way to achieve whatever is expected here?
I'm not sure, though, what is that we try to simulate with this separate thread...

Would you mind to elaborate the logic?

Thanks

[joshiste]

I added this to close the connection after 3 seconds so that the isValid() method returns false and the connection is renewed. Do you now a better way to wait and close the connection?

[artembilan]

Right. 3 seconds is too much for our tests to perform.
We have like 5000 tests in the project. Now imagine if we don't care about the time on them.
How about just to have a connection to be closed immediately on a first call, so the next one would have it re-opened?
Instead of new thread and 3 seconds delay just have an AtomicBoolean flag.

[joshiste]

I don't think using an atomic flag would work here ... As I want the same connection to first succeed on the LISTEN call and then fail after some time. I most probably don't get your suggestion, cause I don't see how to achieve this with an AtomicBoolean....

[artembilan]

Just changed your code to this and it still works:

		AtomicBoolean connectionCloseState = new AtomicBoolean();
		connectionSupplier.onGetConnection = conn -> {
			connectionLatch.countDown();
			if (connectionCloseState.compareAndSet(false, true)) {
				try {
					conn.close();
				}
				catch (Exception e) {
					//nop
				}
			}
		};

What do I miss?

[joshiste]

Does this conn.isValid(1) still report OK in case of DB failover ?

In most cases it will report false, but if it would return true it would be ok to not renew the connection. From the JDBC javadoc:

Returns true if the connection has not been closed and is still valid. The driver shall submit a query on the connection or use some other mechanism that positively verifies the connection is still valid when this method is called.
The query submitted by the driver to validate the connection shall be executed in the context of the current transaction.

The Idea behind this PR is to renew the connection only when we need to.

[artembilan]

Right. 3 seconds is too much for our tests to perform.
We have like 5000 tests in the project. Now imagine if we don't care about the time on them.
How about just to have a connection to be closed immediately on a first call, so the next one would have it re-opened?
Instead of new thread and 3 seconds delay just have an AtomicBoolean flag.

[artembilan]

If you are OK with my change to the test, I can merge it since I have it locally.
Thanks

[artembilan]

Just changed your code to this and it still works:

		AtomicBoolean connectionCloseState = new AtomicBoolean();
		connectionSupplier.onGetConnection = conn -> {
			connectionLatch.countDown();
			if (connectionCloseState.compareAndSet(false, true)) {
				try {
					conn.close();
				}
				catch (Exception e) {
					//nop
				}
			}
		};

What do I miss?

[artembilan]

OK. Since you don't answer, I'm merging this with my correction.
We have to release today.
Feel free to follow up with other possible improvements afterwards.
Thanks

[joshiste]

OK. Since you don't answer, I'm merging this with my correction. We have to release today. Feel free to follow up with other possible improvements afterwards. Thanks

Ah yes sorry, was quite busy. Thanks for merging!