eclipse-vertx / vertx-sql-client

High performance reactive SQL Client written in Java
Apache License 2.0
885 stars 198 forks source link

Oracle Connection Health Check not implemented #1450

Closed weberk closed 2 weeks ago

weberk commented 1 month ago

https://github.com/eclipse-vertx/vertx-sql-client/blob/3417c5ab1e346f852a7b9ae99e9a7d71637dbac2/vertx-oracle-client/src/main/java/io/vertx/oracleclient/impl/OracleJdbcConnection.java#L106 io.vertx.oracleclient.impl.OracleJdbcConnection.isValid() returns allways true.

  @Override
  public boolean isValid() {
    return true;
  }

Please implement by delegating to the oracle.jdbc.OracleConnection. So calling connection.isValid() you get the JDBC4 compliant result the Oracle JDBC driver delivers, which is much. more efficient than any alternative i.e. calling some table called dual!

  @Override
  public boolean isValid() {
    return connection.isValid(oracle.jdbc.OracleConnection.ConnectionValidation.SOCKET, 0);
  }

In order to speedup health check using isValid() it might be good to put the following property to the defaults:

quarkus.datasource.reactive.additional-properties.connectionProperties=oracle.jdbc.defaultConnectionValidation=SOCKET
tsegismont commented 1 month ago

We can't call JDBC connection.isValid() here because it's blocking call. Besides, the expectation from the caller is only to check if the TCP connection is still open.

We have talked in the past about connection validation mechanism when borrowing from the pool, but I can't find any issue.

Does that ring a bell @vietj ?

weberk commented 1 month ago

Calling oracle.jdbc.OracleConnection.isValid() is not always blocking. At least with ConnectionValidation=SOCKET io.vertx.oracleclient.impl.OracleJdbcConnection.isVali(), will do a quick check of the socket status with the Linux kernel. No remote (blocking) network activity is triggered. Using lightweight connection validation overcomes a limitation of the Java Runtime Environment, which does not consume Linux signals raised on TCP-FIN events happening on the network. While the Linux kernel knows the connection is closed, the Java code remains unaware. Consequently, the connection pool might hand out a dead connection, even though it could have been replaced, and some (weak) business logic error handlers are not executed. Resilience is improved when isValid() is called.

tsegismont commented 1 month ago

I read the Lightweight Connection Validation section of the docs.

Indeed, it seems that connection.isValid(oracle.jdbc.OracleConnection.ConnectionValidation.SOCKET, 0); will do.

However, it doesn't seem necessary to set ther oracle.jdbc.defaultConnectionValidation connection property, given that the value is specified in the method call.

tsegismont commented 1 month ago

@weberk I've sent #1451 and asked an Oracle specialist some advice. When we're sure about this, the PR can be merged and the changes backported to the 4.x branch.

weberk commented 1 month ago

Thanks for fetching this Thomas! You are right 'it is not necessary to set ther oracle.jdbc.defaultConnectionValidation connection property, given that the value specified in the method call is overruling the default.'

tsegismont commented 2 weeks ago

@weberk as discussed in #1451, we can't use the SOCKET setting because it may cause the driver to block. In order to provide an experience (and performance) similar to what we have in other clients, we'll use NONE (simple connection lifecycle check).

Combined with a pool max lifetime or idle timeout setting, this should make using the Oracle pool more robust.

tsegismont commented 2 weeks ago

Fixed by f08bfedb and d08f374a14cab847fd885572ea11e037496256b6

weberk commented 2 weeks ago

Thanks for adding any check! Performance is fine, but resilience is also valuable. So now we have a compromise with value from both sides. Thanks.