Skip to content

Conversation

@vitaly-block
Copy link
Collaborator

@vitaly-block vitaly-block commented Oct 7, 2025

Hoist hibernate-like exception handling to misk-jdbc to cover other providers with the same logic

Description

Refactored the TransactionRetryHandler to make database exception retry logic specific to the database type being used.
Database-Specific Retry Logic: Retryable exceptions are now only checked for the configured database type:

  • Connection closed exceptions remain retryable for all databases
  • Vitess-specific exceptions only retry when DataSourceType.VITESS_MYSQL is configured
  • CockroachDB-specific exceptions only retry when DataSourceType.COCKROACHDB is configured
  • TiDB-specific exceptions only retry when DataSourceType.TIDB is configured

API Changes

New Constructor: Added convenient TransactionRetryHandler(qualifierName, databaseType) constructor for easy database type-specific configuration
Backward Compatibility: Existing constructors and APIs remain unchanged

// Before: All database exceptions checked regardless of database type
isMessageRetryable(th)

// After: Database type-specific checking
isConnectionClosed(th) ||
  (databaseType == DataSourceType.VITESS_MYSQL && isRecoverableVitessException(th)) ||
  (databaseType == DataSourceType.COCKROACHDB && isRecoverableCockroachException(th)) ||
  (databaseType == DataSourceType.TIDB && isRecoverableTidbException(th))

There should be no breaking changes, existing code should work:

// Old (still works)
TransactionRetryHandler("mydb")

// New (recommended)
TransactionRetryHandler("mydb", DataSourceType.VITESS_MYSQL)

Testing Strategy

New test cases included, https://gradle.com/s/7wbp6xzb5nt4q

Checklist

  • I have reviewed this PR with relevant experts and/or impacted teams.
  • I have added tests to have confidence my changes work as expected.
  • I have a rollout plan that minimizes risks and includes monitoring for potential issues.

@vitaly-block vitaly-block marked this pull request as draft October 7, 2025 21:32

private fun isMessageRetryable(th: SQLException) =
isConnectionClosed(th) ||
isVitessTransactionNotFound(th) ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are quite a bit of other retryable message strings that we may want to include here that are in our JSC component. Perhaps worth a follow-up PR if we want to keep this PR simpler

@vitaly-block vitaly-block force-pushed the SRE-627-match-hibernate-exception-handling-in-jooq branch from 2b8183c to f6cdea1 Compare October 9, 2025 18:26
@vitaly-block vitaly-block force-pushed the SRE-627-match-hibernate-exception-handling-in-jooq branch from f104796 to 2319b9a Compare October 21, 2025 16:49
@vitaly-block vitaly-block force-pushed the SRE-627-match-hibernate-exception-handling-in-jooq branch from 7f535aa to 46eb183 Compare October 21, 2025 19:48
@vitaly-block vitaly-block force-pushed the SRE-627-match-hibernate-exception-handling-in-jooq branch from 46eb183 to 9a0e5ab Compare October 21, 2025 20:43
@@ -0,0 +1,7 @@
distributionBase=GRADLE_USER_HOME
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be checked in, your local setup seems broken since it should be using Hermit?

gradlew Outdated
@@ -0,0 +1,251 @@
#!/bin/sh
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete pls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants