-
Notifications
You must be signed in to change notification settings - Fork 12
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
Flat queries #487
Flat queries #487
Conversation
jenkins retest |
jenkins retest |
)(_ => IO(DBs.close(dbName.name))) | ||
def resource(liquibaseConfig: LiquibaseConfig, dbName: DatabaseName): Resource[IO, DbReference] = { | ||
for { | ||
dbExecutionContext <- ExecutionContexts.fixedThreadPool[IO](DBs.config.getInt(s"db.${dbName.name.name}.poolMaxSize")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note this change moves the thread pool associated to a db connection internal to the db ref.
private def persistResource(resourceType: ResourceType, resourceId: ResourceId, policies: Set[ValidatableAccessPolicy], authDomain: Set[WorkbenchGroupName], parentOpt: Option[FullyQualifiedResourceId], samRequestContext: SamRequestContext) = | ||
for { | ||
resource <- accessPolicyDAO.createResource(Resource(resourceType.name, resourceId, authDomain, parent = parentOpt), samRequestContext) | ||
policies <- policies.toList.traverse(p => createOrUpdatePolicy(FullyQualifiedPolicyId(resource.fullyQualifiedId, p.policyName), p, samRequestContext)) | ||
} yield resource.copy(accessPolicies = policies.toSet) | ||
private def persistResource(resourceType: ResourceType, resourceId: ResourceId, policies: Set[ValidatableAccessPolicy], authDomain: Set[WorkbenchGroupName], parentOpt: Option[FullyQualifiedResourceId], samRequestContext: SamRequestContext) = { | ||
val accessPolicies = policies.map(constructAccessPolicy(resourceType, resourceId, _, public = false)) // can't set public at create time | ||
accessPolicyDAO.createResource(Resource(resourceType.name, resourceId, authDomain, accessPolicies = accessPolicies, parent = parentOpt), samRequestContext) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to create a resource plus all policies in a single transaction
|
||
_ <- policiesToDelete.toList.parTraverse(p => accessPolicyDAO.deletePolicy(p.id, samRequestContext)).unsafeToFuture() | ||
_ <- maybeDeleteResource(resource, samRequestContext) | ||
_ <- accessPolicyDAO.deleteAllResourcePolicies(resource, samRequestContext).unsafeToFuture() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete all policies in a single transaction
val groupCount = 40 | ||
val userCount = 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scaled down for the sake of elapsed time of test
policy.members.toList.parTraverse { | ||
policy.members.toList.traverse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given the small database there are too many serializable failures with parTraverse
so it turns out to be slower
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking, mostly just questions and nits
...ain/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20201123_effective_policy.xml
Outdated
Show resolved
Hide resolved
src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/RegistrationDAO.scala
Show resolved
Hide resolved
...ala/org/broadinstitute/dsde/workbench/sam/dataAccess/EffectivePolicyMutationStatements.scala
Outdated
Show resolved
Hide resolved
...ala/org/broadinstitute/dsde/workbench/sam/dataAccess/EffectivePolicyMutationStatements.scala
Show resolved
Hide resolved
...ala/org/broadinstitute/dsde/workbench/sam/dataAccess/EffectivePolicyMutationStatements.scala
Show resolved
Hide resolved
src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala
Outdated
Show resolved
Hide resolved
src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala
Show resolved
Hide resolved
src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala
Outdated
Show resolved
Hide resolved
src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala
Outdated
Show resolved
Hide resolved
...ala/org/broadinstitute/dsde/workbench/sam/dataAccess/EffectivePolicyMutationStatements.scala
Show resolved
Hide resolved
...ain/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20201123_effective_policy.xml
Show resolved
Hide resolved
...ain/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20201123_effective_policy.xml
Show resolved
Hide resolved
...ain/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20201123_effective_policy.xml
Outdated
Show resolved
Hide resolved
...ain/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20201123_effective_policy.xml
Outdated
Show resolved
Hide resolved
...ain/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20201123_effective_policy.xml
Outdated
Show resolved
Hide resolved
src/main/scala/org/broadinstitute/dsde/workbench/sam/util/DatabaseSupport.scala
Show resolved
Hide resolved
@@ -137,7 +136,7 @@ object TestSupport extends TestSupport { | |||
|
|||
def genSamRoutesWithDefault(implicit system: ActorSystem, materializer: Materializer): SamRoutes = genSamRoutes(genSamDependencies(), UserInfo(OAuth2BearerToken(""), genWorkbenchUserId(System.currentTimeMillis()), defaultUserEmail, 3600)) | |||
|
|||
lazy val dbRef = DbReference.init(config.as[LiquibaseConfig]("liquibase"), DatabaseNames.Foreground) | |||
lazy val dbRef = DbReference.init(config.as[LiquibaseConfig]("liquibase"), DatabaseNames.Read, TestSupport.blockingEc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be readDbRef
maybe? I'm looking at line 142 below and we're using the read pool to truncate the database. In test world, this is fine, but things might start getting confusing in the tests if we're using read and write pools willy nilly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In unit tests there really is not a difference between read and write pools. Ideally I would not even have it. But I also want to have DatabaseNames
enum and DbReference.init
to use it. So the situation is a little messy and I favor having more mess on the test side than the production side (i.e. I don't want to add a new database name just for tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leave a comment explaining this then? I think it is likely for someone to raise a question about this in the future.
src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala
Outdated
Show resolved
Hide resolved
src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala
Show resolved
Hide resolved
src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala
Show resolved
Hide resolved
...ain/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20201123_effective_policy.xml
Show resolved
Hide resolved
from SAM_EFFECTIVE_RESOURCE_POLICY ep | ||
join SAM_POLICY_ROLE pr on ep.source_policy_id = pr.resource_policy_id | ||
join SAM_RESOURCE resource on ep.resource_id = resource.id | ||
join sam_flattened_role flat_role on pr.resource_role_id = flat_role.base_role_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Out of curiosity, should sam_flattened_role
be in all caps here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't matter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it doesn't matter, would you mind moving it to caps for consistency?
...esources/org/broadinstitute/dsde/sam/liquibase/changesets/20201123_flat_group_membership.xml
Show resolved
Hide resolved
...ala/org/broadinstitute/dsde/workbench/sam/dataAccess/EffectivePolicyMutationStatements.scala
Outdated
Show resolved
Hide resolved
...ala/org/broadinstitute/dsde/workbench/sam/dataAccess/EffectivePolicyMutationStatements.scala
Show resolved
Hide resolved
...ala/org/broadinstitute/dsde/workbench/sam/dataAccess/EffectivePolicyMutationStatements.scala
Outdated
Show resolved
Hide resolved
* F | T | F | F | ||
* F | F | T | F | ||
* F | F | F | T | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I am understanding the logic behind this table, it is:
I xor not(PR or FR)
If so, that may be a useful thing to put in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may also be a simpler way to write the query? If that is the logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marctalbott figured this out, if there were XOR in SQL it might be better but there isn't :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the query looks good, but I was wondering about putting it above the truth table in the comment, just to make it more clear at a first glance. Although since we can't represent the sql that way, I'm okay with leaving it for now.
src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala
Outdated
Show resolved
Hide resolved
src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala
Show resolved
Hide resolved
src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala
Show resolved
Hide resolved
jenkins retest - strange ivy thing on rawls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM at this point, don't think any of my nits should hold this up.
* F | T | F | F | ||
* F | F | T | F | ||
* F | F | F | T | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the query looks good, but I was wondering about putting it above the truth table in the comment, just to make it more clear at a first glance. Although since we can't represent the sql that way, I'm okay with leaving it for now.
...ain/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20201123_effective_policy.xml
Show resolved
Hide resolved
from SAM_EFFECTIVE_RESOURCE_POLICY ep | ||
join SAM_POLICY_ROLE pr on ep.source_policy_id = pr.resource_policy_id | ||
join SAM_RESOURCE resource on ep.resource_id = resource.id | ||
join sam_flattened_role flat_role on pr.resource_role_id = flat_role.base_role_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it doesn't matter, would you mind moving it to caps for consistency?
👍 |
jenkins retest |
Ticket: https://broadworkbench.atlassian.net/browse/CA-315
org/broadinstitute/dsde/workbench/sam/dataAccess
and moved most of the DAOs therePR checklist