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

[AN-286] Fixes deadlock issue in GROUP_METRICS_ENTRY table #7677

Merged
merged 10 commits into from
Jan 16, 2025

Conversation

salonishah11
Copy link
Contributor

Description

Fixes the deadlock issue in GROUP_METRICS_ENTRY table.

Jira ticket: https://broadworkbench.atlassian.net/browse/AN-286

Release Notes Confirmation

CHANGELOG.md

  • I updated CHANGELOG.md in this PR
  • I assert that this change shouldn't be included in CHANGELOG.md because it doesn't impact community users

Terra Release Notes

  • I added a suggested release notes entry in this Jira ticket
  • I assert that this change doesn't need Jira release notes because it doesn't impact Terra users

@salonishah11 salonishah11 requested a review from a team as a code owner January 14, 2025 00:12
Copy link
Collaborator

@aednichols aednichols left a comment

Choose a reason for hiding this comment

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

Very cool! 👍

Walkthrough worthy IMO, I would love to hear your process.

Comment on lines +49 to +51
if ex.isInstanceOf[SQLIntegrityConstraintViolationException] || (ex
.isInstanceOf[PSQLException] && ex.getMessage.contains(
"duplicate key value violates unique constraint \"UC_GROUP_METRICS_ENTRY_GI\""
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are different cases for various DBs? I assume PSQL... is Postgres.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is correct. Postgres throws general PSQLException when duplicate values are inserted and hence I added the text checking as well here.

}

/*
The approach here is to try and insert the record into the table and if that fails because a record with that
Copy link
Contributor

Choose a reason for hiding this comment

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

Crystal clear explanation, thanks a lot for adding that 😻

} yield ()
runTransaction(action)

runTransaction(insertAction, TransactionIsolation.ReadCommitted)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add the link to the doc stating that postgress recommends ReaCommitted?

@@ -109,6 +109,34 @@ class GroupMetricsSlickDatabaseSpec extends AnyFlatSpec with Matchers with Scala
} yield ()).futureValue
}

it should "handle parallel upserts to database" taggedAs DbmsTest in {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test!

@salonishah11 salonishah11 merged commit a00a190 into develop Jan 16, 2025
42 checks passed
@salonishah11 salonishah11 deleted the sps_group_metrics_deadlock branch January 16, 2025 20:26
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.

3 participants