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.

Sign up for GitHub

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

ID-896 Isolate Misbehaving Query #1230

Merged
merged 3 commits into from
Oct 26, 2023
Merged

ID-896 Isolate Misbehaving Query #1230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
20e44da
ID-896 Isolate Misbehaving Query
tlangs Oct 26, 2023
2f05ba7
rename and fix bug
tlangs Oct 26, 2023
dac6994
more renaming
tlangs Oct 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/main/resources/sam.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ db {
poolMaxSize = 8
poolConnectionTimeoutMillis = 5000
driver = "org.postgresql.Driver"
url = ${?POSTGRES_READ_URL}
url = ${?POSTGRES_WRITE_URL}
user = ${?POSTGRES_USERNAME}
password = ${?POSTGRES_PASSWORD}
}
Expand Down Expand Up @@ -162,6 +162,17 @@ db {
user = ${?POSTGRES_USERNAME}
password = ${?POSTGRES_PASSWORD}
}

sam_read_replica {
poolName = "sam_read_replica"
poolInitialSize = 8
poolMaxSize = 8
poolConnectionTimeoutMillis = 5000
driver = "org.postgresql.Driver"
url = ${?POSTGRES_READ_URL}
user = ${?POSTGRES_USERNAME}
password = ${?POSTGRES_PASSWORD}
}
}

admin {
Expand Down
10 changes: 7 additions & 3 deletions src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,16 @@ object Boot extends IOApp with LazyLogging {
(foregroundDirectoryDAO, foregroundAccessPolicyDAO, postgresDistributedLockDAO, azureManagedResourceGroupDAO, lastQuotaErrorDAO) <- createDAOs(
appConfig,
appConfig.samDatabaseConfig.samWrite,
appConfig.samDatabaseConfig.samRead
appConfig.samDatabaseConfig.samRead,
appConfig.samDatabaseConfig.samReadReplica
)

// This special set of objects are for operations that happen in the background, i.e. not in the immediate service
// of an api call (foreground). They are meant to partition resources so that background processes can't crowd our api calls.
(backgroundDirectoryDAO, backgroundAccessPolicyDAO, _, _, _) <- createDAOs(
appConfig,
appConfig.samDatabaseConfig.samBackground,
appConfig.samDatabaseConfig.samBackground,
appConfig.samDatabaseConfig.samBackground
)

Expand Down Expand Up @@ -201,14 +203,16 @@ object Boot extends IOApp with LazyLogging {
private def createDAOs(
appConfig: AppConfig,
writeDbConfig: DatabaseConfig,
readDbConfig: DatabaseConfig
readDbConfig: DatabaseConfig,
readReplicaDbConfig: DatabaseConfig
): cats.effect.Resource[IO, (PostgresDirectoryDAO, AccessPolicyDAO, PostgresDistributedLockDAO[IO], AzureManagedResourceGroupDAO, LastQuotaErrorDAO)] =
for {
writeDbRef <- DbReference.resource(appConfig.liquibaseConfig, writeDbConfig)
readDbRef <- DbReference.resource(appConfig.liquibaseConfig.copy(initWithLiquibase = false), readDbConfig)
readReplicaDbRef <- DbReference.resource(appConfig.liquibaseConfig.copy(initWithLiquibase = false), readReplicaDbConfig)

directoryDAO = new PostgresDirectoryDAO(writeDbRef, readDbRef)
accessPolicyDAO = new PostgresAccessPolicyDAO(writeDbRef, readDbRef)
accessPolicyDAO = new PostgresAccessPolicyDAO(writeDbRef, readDbRef, Some(readReplicaDbRef))
postgresDistributedLockDAO = new PostgresDistributedLockDAO[IO](writeDbRef, readDbRef, appConfig.distributedLockConfig)
azureManagedResourceGroupDAO = new PostgresAzureManagedResourceGroupDAO(writeDbRef, readDbRef)
lastQuotaErrorDAO = new PostgresLastQuotaErrorDAO(writeDbRef, readDbRef)
Expand Down
7 changes: 6 additions & 1 deletion src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,12 @@ object AppConfig {
}

implicit val samDatabaseConfigReader: ValueReader[SamDatabaseConfig] = ValueReader.relative { config =>
SamDatabaseConfig(config.as[DatabaseConfig]("sam_read"), config.as[DatabaseConfig]("sam_write"), config.as[DatabaseConfig]("sam_background"))
SamDatabaseConfig(
config.as[DatabaseConfig]("sam_read"),
config.as[DatabaseConfig]("sam_write"),
config.as[DatabaseConfig]("sam_background"),
config.as[DatabaseConfig]("sam_read_replica")
)
}

final case class AdminConfig(superAdminsGroup: WorkbenchEmail, allowedEmailDomains: Set[String], serviceAccountAdmins: Set[WorkbenchEmail])
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/org/broadinstitute/dsde/workbench/sam/config/DatabaseConfig.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.broadinstitute.dsde.workbench.sam.config

final case class SamDatabaseConfig(samRead: DatabaseConfig, samWrite: DatabaseConfig, samBackground: DatabaseConfig)
final case class SamDatabaseConfig(samRead: DatabaseConfig, samWrite: DatabaseConfig, samBackground: DatabaseConfig, samReadReplica: DatabaseConfig)

final case class DatabaseConfig(
dbName: Symbol,
Expand Down
8 changes: 6 additions & 2 deletions ...main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ import scala.collection.concurrent.TrieMap
import scala.util.{Failure, Try}
import cats.effect.Temporal

class PostgresAccessPolicyDAO(protected val writeDbRef: DbReference, protected val readDbRef: DbReference)(implicit timer: Temporal[IO])
class PostgresAccessPolicyDAO(
protected val writeDbRef: DbReference,
protected val readDbRef: DbReference,
protected override val readReplicaDbRef: Option[DbReference] = None
)(implicit timer: Temporal[IO])
extends AccessPolicyDAO
with DatabaseSupport
with PostgresGroupDAO
Expand Down Expand Up @@ -1353,7 +1357,7 @@ class PostgresAccessPolicyDAO(protected val writeDbRef: DbReference, protected v
userId: WorkbenchUserId,
samRequestContext: SamRequestContext
): IO[Iterable[ResourceIdWithRolesAndActions]] =
readOnlyTransaction("listUserResourcesWithRolesAndActions", samRequestContext) { implicit session =>
readOnlyTransactionReadReplica("listUserResourcesWithRolesAndActions", samRequestContext) { implicit session =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you want to do this for all usages of UserResourcesQuery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. The query that's been blowing up the DB seems to be the one with as inherited in it. Being as surgical as possible seems like the right move here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote that given where we are with this, we isolate this one major offender first. 80/20 rule. We might be able to make things a lot better at some point, but for now we just need to treat the most severe problem.

class ListUserResourcesQuery extends UserResourcesQuery(resourceTypeName, None, userId) {
val policyRole = EffectivePolicyRoleTable.syntax("policyRole")
val resourceRole = ResourceRoleTable.syntax("resourceRole")
Expand Down
7 changes: 7 additions & 0 deletions src/main/scala/org/broadinstitute/dsde/workbench/sam/util/DatabaseSupport.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,19 @@ import cats.effect.Temporal
trait DatabaseSupport {
protected val writeDbRef: DbReference
protected val readDbRef: DbReference
protected val readReplicaDbRef: Option[DbReference] = None

protected def readOnlyTransaction[A](dbQueryName: String, samRequestContext: SamRequestContext)(databaseFunction: DBSession => A): IO[A] = {
val databaseIO = IO(readDbRef.readOnly(databaseFunction))
readDbRef.runDatabaseIO(dbQueryName, samRequestContext, databaseIO)
}

protected def readOnlyTransactionReadReplica[A](dbQueryName: String, samRequestContext: SamRequestContext)(databaseFunction: DBSession => A): IO[A] = {
val dbRef = readReplicaDbRef.getOrElse(readDbRef)
val databaseIO = IO(dbRef.readOnly(databaseFunction))
readDbRef.runDatabaseIO(dbQueryName, samRequestContext, databaseIO)
}

/** Run a database transaction with isolation level set to serializable. See https://www.postgresql.org/docs/9.6/transaction-iso.html. Use this when you need
* to be sure there are no concurrent changes that affect the outcome of databaseFunction. When a transaction fails due to a serialization failure it may be
* retried.
Expand Down
Loading