Allow JDBC connectors to retry on transient failures #19191

dominikzalewski · 2023-09-28T16:57:58Z

Description

RetryingConnectionFactory now has a pluggable JDBC-connector specific retry strategy. This PR applies this change only to Oracle.

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text:
All JDBC-based connectors are now always retrying (5 times for 30 seconds with a backoff) if opening a connection fails. By default this happens only if SQLTransientException is encountered. For backward compatibility, for Oracle this is overridden to SQLRecoverableException.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/RetryingConnectionFactory.java

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClientModule.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/RetryingConnectionFactoryModule.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/RetryingConnectionFactory.java

hashhar

some comments

right now the refactors look larger than the actual change and I can't be sure what is refactor vs what is required for the functionality

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClientModule.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/RetryingConnectionFactoryModule.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestRetryingConnectionFactory.java

hashhar · 2023-10-03T11:43:03Z

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClientModule.java

+            if (oracleConfig.isConnectionPoolEnabled()) {
+                return oraclePoolConnectionFactory;
+            }
+            return retryingConnectionFactory;


looks like pre-exisiting logic that we only use RCF with non-pooled connections.
I don't know if it was intentional or not, so either we should add RCF to pooled connection factory as well or add a comment explaining why we only use RCF with non-pooled connections.

Sorry, but this ask seems out of scope? This has only been revealed because I slightly refactor the existing code and how it is more evident what the logic is. The scope has been frozen before I started working. If there's a good reason that we should change it, then we will. But we need a good reason.

Acceptance criteria

no new regressions

unit tests for new framework that it actually retries

it is possible to implement retry for any connector in future that fails sometimes to open a connection

no custom retrying in any jdbc connector, all connectors use new framework

Let me know if I misunderstood AC laid out by @kokosing.

dominikzalewski · 2023-10-03T12:21:51Z

right now the refactors look larger than the actual change and I can't be sure what is refactor vs what is required for the functionality

I have broken down the change into a few commits, so that it is clear where the refactor ends and where adding functionality starts. If this is still not helpful, let's have a call.

electrum · 2023-10-03T17:16:56Z

The commit titles are too long. Please see https://cbea.ms/git-commit/

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClientModule.java

dominikzalewski · 2023-10-04T10:16:17Z

The commit titles are too long. Please see https://cbea.ms/git-commit/

Shortened commit messages and formatted the extended description to 72 chars column.

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClientModule.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/RetryingConnectionFactory.java

kokosing · 2023-10-05T11:54:20Z

How about PR title:

Allow JDBC connectors to retry on transient failures

Please provide PR description too and fill RN (release notes) form.

dominikzalewski · 2023-10-11T07:21:51Z

I have also forgot to mention to you before that there is also io.trino.plugin.jdbc.LazyConnectionFactory :) Just wanted to make sure this puzzle is not too simple for you and you remember about all puzzles.

Thanks. I spent some time analyzing this, as your previous hint from slack was actually tracking the JdbcClient chain, rather than ConnectionFactory. But I figured it out by myself and it made me so proud :)

Simplified the chain with Lazy and Stats. There's a separate commit.

kokosing

Just one comment.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/LazyConnectionFactory.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestLazyConnectionFactory.java

kokosing

minor comments

I would like @hashhar to take a look too.

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestRetryingConnectionFactory.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestLazyConnectionFactory.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/RetryingConnectionFactoryModule.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestLazyConnectionFactory.java

Refactor TestRetryingConnectionFactory and TestLazyConnectionFactory to use Dependency Injection Motivation behind this change is to be able to later add dependent objects to tested class without a need for major changes in the test class itself.

hashhar

The impl looks very good. I have no comments about it.

The only comment I have is about inspecting how the Guice map looks after the 2nd commit. From what I can tell we seem to have introduced duplication.

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestRetryingConnectionFactory.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForLazyConnectionFactory.java

hashhar · 2023-10-18T18:51:09Z

For references here's the actual Guice map before and after change for both JdbcClient and ConnectionFactory

BEFORE:

DriverConnectionFactory (OracleClientModule#connectionFactory == @ForBaseJdbc)
RetryingConnectionFactory (RetryingConnectionFactory#ctor)
StatisticsAwareConnectionFactory (StatisticsAwareConnectionFactory#ctor)
LazyConnectionFactory (LazyConnectionFactory#ctor)
ReusableConnectionFactory (ResuableConnectionFactoryModule - conditional, if QueryConfig#isReuseEnabled is true)

------------------------------------------------------------

OracleClient (OracleClientModule#setup == @ForBaseJdbc)
ForwardingJdbcClient (JdbcDiagnosticModule#createJdbcClientWithStats)
StatisticsAwareJdbcClient (JdbcDiagnosticModule#createJdbcClientWithStats == @StatsCollecting)
CachingJdbcClient (CachingJdbcClient#ctor)

AFTER:

DriverConnectionFactory (OracleClientModule#connectionFactory == @ForBaseJdbc)
StatisticsAwareConnectionFactory (StatisticsAwareConnectionFactory#ctor)
RetryingConnectionFactory (RetryingConnectionFactory#ctor)
LazyConnectionFactory (LazyConnectionFactory#ctor)
ReusableConnectionFactory (ResuableConnectionFactoryModule - conditional, if QueryConfig#isReuseEnabled is true)

No change in JdbcClient tree after the change.

comments have been addressed

hashhar · 2023-10-19T12:36:17Z

@dominikzalewski can you please fill in the release notes template in the PR description since this would be a user visible change (even if not directly configurable).

mosabua · 2023-10-21T00:39:06Z

Fyi .. please check out the final release notes entry we added for future references and as an example.

cla-bot bot added the cla-signed label Sep 28, 2023

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch 3 times, most recently from caadc2e to 992a02b Compare September 29, 2023 15:28

kokosing reviewed Oct 1, 2023

View reviewed changes

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch 8 times, most recently from a04c427 to fd98944 Compare October 3, 2023 08:30

dominikzalewski changed the title ~~[WiP] Refactor to use DI instead of constructor calls~~ [WiP] RetryingConnectionFactory has strategy of retries externalized Oct 3, 2023

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch from fd98944 to 4f040f7 Compare October 3, 2023 08:50

dominikzalewski changed the title ~~[WiP] RetryingConnectionFactory has strategy of retries externalized~~ RetryingConnectionFactory has strategy of retries externalized Oct 3, 2023

dominikzalewski marked this pull request as ready for review October 3, 2023 09:58

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch from 4f040f7 to f7ff814 Compare October 3, 2023 11:25

hashhar reviewed Oct 3, 2023

View reviewed changes

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch 2 times, most recently from 5266786 to 0ae9875 Compare October 3, 2023 12:10

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch from 0ae9875 to dce2564 Compare October 3, 2023 13:42

electrum previously requested changes Oct 3, 2023

View reviewed changes

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClientModule.java Outdated Show resolved Hide resolved

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch from dce2564 to 2c62916 Compare October 4, 2023 10:14

kokosing reviewed Oct 5, 2023

View reviewed changes

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch from 9ad8864 to f2c32b0 Compare October 11, 2023 07:18

dominikzalewski marked this pull request as ready for review October 11, 2023 07:25

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch from 8638db0 to 9244f8c Compare October 11, 2023 08:26

kokosing reviewed Oct 11, 2023

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/LazyConnectionFactory.java Outdated Show resolved Hide resolved

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch 5 times, most recently from 14167f5 to 24e316a Compare October 17, 2023 11:01

dominikzalewski commented Oct 17, 2023

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/LazyConnectionFactory.java Outdated Show resolved Hide resolved

dominikzalewski commented Oct 17, 2023

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestLazyConnectionFactory.java Outdated Show resolved Hide resolved

kokosing reviewed Oct 17, 2023

View reviewed changes

hashhar self-requested a review October 17, 2023 12:35

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch from 24e316a to 0f67a87 Compare October 17, 2023 15:46

dominikzalewski added 3 commits October 17, 2023 17:53

Simplify ConnectionFactory chain of responsibility

1206ec6

Pluggable retry strategy for RetryingConnectionFactory

da3f8ec

dominikzalewski force-pushed the retry_on_jdbc_open_connection branch from 0f67a87 to da3f8ec Compare October 17, 2023 15:54

hashhar reviewed Oct 18, 2023

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestRetryingConnectionFactory.java Show resolved Hide resolved

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForLazyConnectionFactory.java Show resolved Hide resolved

hashhar approved these changes Oct 19, 2023

View reviewed changes

hashhar merged commit 74012e3 into trinodb:master Oct 19, 2023
60 checks passed

github-actions bot added this to the 430 milestone Oct 19, 2023

dominikzalewski deleted the retry_on_jdbc_open_connection branch October 19, 2023 12:40

dominikzalewski mentioned this pull request Oct 20, 2023

Update DEVELOPMENT.md: Git merge strategy #19352

Merged

mosabua mentioned this pull request Oct 20, 2023

Add Trino 430 release notes #19428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow JDBC connectors to retry on transient failures #19191

Allow JDBC connectors to retry on transient failures #19191

dominikzalewski commented Sep 28, 2023 •

edited

Loading

hashhar left a comment

hashhar Oct 3, 2023

dominikzalewski Oct 3, 2023

dominikzalewski commented Oct 3, 2023

electrum commented Oct 3, 2023

dominikzalewski commented Oct 4, 2023

kokosing commented Oct 5, 2023

dominikzalewski commented Oct 11, 2023

kokosing left a comment

kokosing left a comment

hashhar left a comment

hashhar commented Oct 18, 2023

hashhar commented Oct 19, 2023

mosabua commented Oct 21, 2023

Allow JDBC connectors to retry on transient failures #19191

Allow JDBC connectors to retry on transient failures #19191

Conversation

dominikzalewski commented Sep 28, 2023 • edited Loading

Description

Release notes

hashhar left a comment

Choose a reason for hiding this comment

hashhar Oct 3, 2023

Choose a reason for hiding this comment

dominikzalewski Oct 3, 2023

Choose a reason for hiding this comment

dominikzalewski commented Oct 3, 2023

electrum commented Oct 3, 2023

dominikzalewski commented Oct 4, 2023

kokosing commented Oct 5, 2023

dominikzalewski commented Oct 11, 2023

kokosing left a comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

hashhar commented Oct 18, 2023

hashhar commented Oct 19, 2023

mosabua commented Oct 21, 2023

dominikzalewski commented Sep 28, 2023 •

edited

Loading