Validate connections before using them and expose validation as config. #249

briancecker · 2019-08-27T22:35:32Z

isClosed is not sufficient to validate connections. You need to check that they are actually still usable, which is what isValid does. This PR prevents applications from seeing issues when databases failover and connections are no longer valid.

briancecker · 2019-08-27T22:38:51Z

jack-core/src/com/rapleaf/jack/DatabaseConnectionConfiguration.java

-    return new DatabaseConnectionConfiguration(adapter, host, dbName, port, parallelTesting, username, password);
+    Optional<Integer> connectionMaxRetries = loadOpt(dbInfo, "connection_max_retries",
+        envVar(CONNECTION_MAX_RETRIES, dbNameKey), prop(CONNECTION_MAX_RETRIES, dbNameKey), new ToLong())
+        .transform(Long::intValue);


This is super annoying, but you need to explicitly convert these to ints like this because the cast at https://github.com/LiveRamp/jack/blob/master/jack-core/src/com/rapleaf/jack/DatabaseConnectionConfiguration.java#L164 is not sufficient, and the underlying type is a Long, not an Integer because the YAML loader reads all numbers as Longs. The only reason we don't see this issue with the port config as well is because we never actually treat it as an int later on.

Without doing this, we'll get a ClassCastException in DatabaseConnection at this line: https://github.com/LiveRamp/jack/pull/249/files#diff-bb343e2007095498d574a0c34e39f6c9R98

Thanks for the explanation here!

👍 Good explanation. Could you add some of that as a comment? I think I'd have a hard time reconstructing that just from the code

Sure, I added the comment.

bvivio

LGTM % a few nits. You'll probably want to get some other eyes on this from #dev-cat (in other words, I don't trust myself to be a primary reviewer on jack code).

FYI @pwestling is no longer with us at LiveRamp...

jack-core/src/com/rapleaf/jack/DatabaseConnection.java

jack-core/src/com/rapleaf/jack/DatabaseConnectionConfiguration.java

bvivio · 2019-08-27T22:55:41Z

jack-core/src/com/rapleaf/jack/DatabaseConnectionConfiguration.java

-    return new DatabaseConnectionConfiguration(adapter, host, dbName, port, parallelTesting, username, password);
+    Optional<Integer> connectionMaxRetries = loadOpt(dbInfo, "connection_max_retries",
+        envVar(CONNECTION_MAX_RETRIES, dbNameKey), prop(CONNECTION_MAX_RETRIES, dbNameKey), new ToLong())
+        .transform(Long::intValue);


Thanks for the explanation here!

jack-core/src/com/rapleaf/jack/DatabaseConnection.java

briancecker · 2019-08-27T23:30:53Z

Unfortunately, this PR doesn't address connections that are pooled by the DbPoolManager, but to do that, I'd have to implement https://github.com/LiveRamp/jack/blob/master/jack-core/src/com/rapleaf/jack/transaction/DbPoolManager.java#L174, which would require me to add some sort of validate method on IDb as well, but that would be quite a breaking change for many downstream applications.

The other option would be to implement validateObject as just a call to connection.getObject().setAutoCommit(true); or something like that...

Edit: So it does actually work. At some point DatabaseConnection.getConnectionInternal is getting called anyways, even when using the DbPoolManager, so the check for unusable connections added in this PR still gets triggered, and the retries in DatabaseConnection.getConnectionInternal fire. I did also find a way to implement DbPoolManager.DbPoolFactory.validateObject though. Not sure if it's necessary, since it eventually gets to the lower level check, but you can do it like:

public boolean validateObject(PooledObject<DB> connection) {
      try {
        Records records = connection.getObject()
            .findBySql("SELECT 1 as id",
                Collections.emptyList(),
                Collections.singletonList(Column.fromId(null))
            );
        return records.size() == 1;
      } catch (IOException e) {
        return false;
      }
    }

briancecker · 2019-08-28T16:53:55Z

I decided to implement the above comment, which validates connections from the connection pool on borrowing them. The commit message, copied below, explains the logic for doing this:

    This is technically covered by the implementation of most methods in
    `AbstractDatabaseModel`, which check for exceptions and then call
    `resetConnection`, but this explicitly ensures that no matter the
    underlying method implementation, we'll still validate the connection
    before using it. We also validate while connections are idle to reduce
    the number of destroy/create cycles that we might need at the time of
    borrowing a connection. This is especially relevant for long running
    processes that might attempt to use very old pooled connections.

    Finally, common-pool2 was upgraded to 2.7.0 to fix a bug in validation
    logic (https://github.com/apache/commons-pool/pull/23).

tuliren · 2019-08-28T18:29:05Z

jack-core/pom.xml

@@ -71,7 +71,7 @@
    <dependency>
      <groupId>org.apache.commons</groupId>
      <artifactId>commons-pool2</artifactId>
-      <version>2.4.2</version>
+      <version>2.7.0</version>


FYI, there is an issue in commons-pool2 described here: #195 (comment)

I roughly remembered that the root cause is that the new timer object is not run as a daemon thread and can only be shutdown manually, which will block workflow termination. Not sure if the issue has been addressed in 2.7.0. I highly doubt that.

Although the object pool should be explicitly closed in general / as a best practice, but when I left LiveRamp, most of the workflows don't do that. Also when the object pool was introduced to jack, one of the goal was to shield user from having to worry about closing and releasing the resource.

Hmm, okay. I will put it back to 2.4.2 then. The bug which caused me to update this is not so serious, it just means we validate the connections on create as well as borrow.

Any other feedback, or does this look good from your side @tuliren? You seem like the correct person to ask considering you have the most commits to this repo ;)

Looks good.

I guess maybe there should be a comment here that warns people about the commons-pool2 version issue. Can you add that with this PR? Thanks.

Added the comment. Thanks for the feedback.

This is technically covered by the implementation of most methods in `AbstractDatabaseModel`, which check for exceptions and then call `resetConnection`, but this explicitly ensures that no matter the underlying method implementation, we'll still validate the connection before using it. We also validate while connections are idle to reduce the number of destroy/create cycles that we might need at the time of borrowing a connection. This is especially relevant for long running processes that might attempt to use very old pooled connections.

briancecker commented Aug 27, 2019

View reviewed changes

briancecker requested a review from bvivio August 27, 2019 22:46

qstearns requested a review from pwestling August 27, 2019 22:49

bvivio approved these changes Aug 27, 2019

View reviewed changes

briancecker force-pushed the validate-connection-on-usage branch from 8b5e3e9 to f1f30c2 Compare August 27, 2019 23:05

Validate connections before using them and expose validation as config.

f790a75

briancecker force-pushed the validate-connection-on-usage branch from f1f30c2 to f790a75 Compare August 28, 2019 00:20

tuliren reviewed Aug 28, 2019

View reviewed changes

briancecker force-pushed the validate-connection-on-usage branch from 18d4a7c to 3077e05 Compare August 28, 2019 22:29

briancecker force-pushed the validate-connection-on-usage branch from 3077e05 to e42c212 Compare August 29, 2019 21:19

briancecker merged commit 6dbd62a into master Aug 30, 2019

delete-merged-branch bot deleted the validate-connection-on-usage branch August 30, 2019 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate connections before using them and expose validation as config. #249

Validate connections before using them and expose validation as config. #249

briancecker commented Aug 27, 2019

briancecker Aug 27, 2019 •

edited

Loading

bvivio Aug 27, 2019

joshcurtis Aug 27, 2019

briancecker Aug 28, 2019

bvivio left a comment

bvivio Aug 27, 2019

briancecker commented Aug 27, 2019 •

edited

Loading

briancecker commented Aug 28, 2019

tuliren Aug 28, 2019 •

edited

Loading

briancecker Aug 28, 2019 •

edited

Loading

briancecker Aug 29, 2019

tuliren Aug 29, 2019

briancecker Aug 29, 2019

tuliren Aug 30, 2019

Validate connections before using them and expose validation as config. #249

Validate connections before using them and expose validation as config. #249

Conversation

briancecker commented Aug 27, 2019

briancecker Aug 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvivio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briancecker commented Aug 27, 2019 • edited Loading

briancecker commented Aug 28, 2019

tuliren Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

briancecker Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briancecker Aug 27, 2019 •

edited

Loading

briancecker commented Aug 27, 2019 •

edited

Loading

tuliren Aug 28, 2019 •

edited

Loading

briancecker Aug 28, 2019 •

edited

Loading