Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DT-1184: update db retry catch to use new exception class #1895

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

pshapiro4broad
Copy link
Member

Jira ticket: https://broadworkbench.atlassian.net/browse/DT-1184

Addresses

If a SQL operation in a Step failed due to a conflict, it would no longer be retried, which was causing integration tests to fail, and could cause a user's flight to fail that would've otherwise succeeded.

Summary of changes

CannotSerializeTransactionException was deprecated in spring framework 6.0.3. At some point, this exception was no longer thrown by the spring SQL layer and code we have that depended on it stopped working.

There are a number of new possible exceptions to catch, but TransientDataAccessException seems like the most general type of exception which indicates a retryable SQL operation.

Testing Strategy

  • unit tests
  • integration tests

@pshapiro4broad pshapiro4broad requested a review from a team as a code owner January 27, 2025 16:47
@pshapiro4broad pshapiro4broad requested review from rushtong and rjohanek and removed request for a team January 27, 2025 16:47
Copy link
Contributor

@rjohanek rjohanek left a comment

Choose a reason for hiding this comment

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

this looks good, thank you!

Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

looks good, thank you 👍

@pshapiro4broad pshapiro4broad enabled auto-merge (squash) January 27, 2025 19:51
Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

Looks reasonable, 👍🏽 once tests are passing.

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