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

Jump to bottom

resources: Customizable contexts. #7678

Merged
merged 16 commits into from
Oct 20, 2020
Merged

resources: Customizable contexts. #7678

merged 16 commits into from
Oct 20, 2020

Conversation

ghost
Copy link

@ghost ghost commented Oct 14, 2020

This wraps the resource acquisition execution context in a type, for a few reasons:

  1. It makes it easier to ensure the execution context does not leak into the main part of the application.
  2. We can more easily identify when a strange execution context is used, such as the DirectExecutionContext.
  3. We can potentially add extra information to this type, such as the logging context.

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

@SamirTalwar
resources: Move builders into //ledger/ledger-resources.
479ec00 
Keep the actual constructors in a trait, but instantiate it when working
with ledger code.

This allows us to later introduce an extra "context" type parameter to
ResourceOwner.
@SamirTalwar
resources-akka: Move the builders in to //ledger/ledger-resources.
4754fcd
@SamirTalwar
resources: Introduce an abstract Context parameter for owners.
c883016 
This replaces the concrete `ExecutionContext`. While it _can_ be an
execution context, it really doesn't matter as long as we can get at one
somehow.

This is being introduced so we can wrap the context in a container,
either for type tagging or to include extra information.

Because our current context _is_ `ExecutionContext`, and an implicit is
provided to extract it, we can end up with two ways to get the same
value. We use shadowing to prevent this. This problem should go away in
the near future when a new context type is added.

CHANGELOG_BEGIN
- [Integration Kit] The `ResourceOwner` type is now parameterized by a
  `Context`, which is filled in by the corresponding `Context` class in
  the _ledger-resources_ dependency. This allows us to pass extra
  information through resource acquisition.
CHANGELOG_END
@SamirTalwar
ledger-resources: Move ResourceOwner here from resources.
ac3999d
@SamirTalwar
ledger-resources: Remove dependencies from outside //ledger.
3dc4329
@SamirTalwar
ledger-resource: Wrap the acquisition execution context in Context.
e44a046 
So we can add a logging context to it.
@SamirTalwar
resources: Pass the Context, not the ExecutionContext, to Resource.
e216161
@SamirTalwar
Avoid importing HasExecutionContext.
60805ab
@ghost ghost requested review from aherrmann-da and cocreature as code owners October 14, 2020 09:12
@ghost ghost self-requested a review October 14, 2020 09:12
@ghost
Copy link
Author

ghost commented Oct 14, 2020

BTW, I tried to make it wrap a phantom-typed execution context (i.e. ExecutionContext[Acquisition]) to make it even harder to leak, but I had to change so much to make it work and it was an uphill struggle due to IntelliJ not being able to recognize "no such method: flatMap" vs. the types not lining up perfectly to use the relevant implicit.

TLDR implicits make everything fun.

stefanobaghino-da
stefanobaghino-da reviewed Oct 14, 2020
View reviewed changes
Copy link
Contributor

@stefanobaghino-da stefanobaghino-da left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I would approve, but this PR is too large in scope to be merged without at least a second review.

ledger/participant-integration-api/src/main/scala/platform/indexer/JdbcIndexer.scala Outdated Show resolved Hide resolved
ledger/participant-integration-api/src/main/scala/platform/indexer/JdbcIndexer.scala
): Future[Unit] = {
private def initOrCheckParticipantId(
dao: LedgerDao,
)(implicit resourceContext: ResourceContext): Future[Unit] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's inconsistency in how you name your implicit resource contexts. I would recommend sticking to the naming you used here (resourceContext), since we also have other contexts we pass around.

Copy link
Author

Choose a reason for hiding this comment

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

I tried to keep it as resourceContext except when it's the implicit parameter of a ResourceOwner#acquire, in which case it's context. Is this too confusing?

ledger/participant-integration-api/src/main/scala/platform/store/FlywayMigrations.scala Show resolved Hide resolved
...participant-integration-api/src/test/lib/scala/platform/store/dao/JdbcLedgerDaoBackend.scala Outdated Show resolved Hide resolved
ledger/participant-integration-api/src/test/suite/scala/platform/apiserver/GrpcServerSpec.scala Outdated Show resolved Hide resolved
libs-scala/resources/src/main/scala/com/digitalasset/resources/AbstractResourceOwner.scala Outdated Show resolved Hide resolved
libs-scala/resources/src/main/scala/com/digitalasset/resources/HasExecutionContext.scala Outdated Show resolved Hide resolved
@stefanobaghino-da
Copy link
Contributor

BTW, I tried to make it wrap a phantom-typed execution context (i.e. ExecutionContext[Acquisition]) to make it even harder to leak, but I had to change so much to make it work and it was an uphill struggle due to IntelliJ not being able to recognize "no such method: flatMap" vs. the types not lining up perfectly to use the relevant implicit.

TLDR implicits make everything fun.

I'm a bit worried this is getting a bit too clever.

SamirTalwar and others added 3 commits October 14, 2020 14:52
@SamirTalwar @stefanobaghino-da
resources: Make the small changes suggested by @stefanobaghino-da.
c3f3c5a 
Co-Authored-By: Stefano Baghino <43749967+stefanobaghino-da@users.noreply.github.com>
@SamirTalwar
ledger-resources: Pull out a trait for test resource contexts.
f378d21 
Saves a few lines of code.
@SamirTalwar
Merge remote-tracking branch 'origin/master' into samir/libs-scala/re…
9079237 
…sources/customizable-contexts
@ghost
Copy link
Author

ghost commented Oct 14, 2020

I'm a bit worried this is getting a bit too clever.

I came to the same conclusion and threw away that particular changeset.

@ghost
Copy link
Author

ghost commented Oct 20, 2020

/azp run digital-asset.daml (macOS)

@azure-pipelines
Copy link
Contributor

No pipelines are associated with this pull request.

@ghost
Copy link
Author

ghost commented Oct 20, 2020

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@stefanobaghino-da
Copy link
Contributor

/azp run digital-asset.daml (macOS)

You wish! 😂

@ghost
Copy link
Author

ghost commented Oct 20, 2020

Yeah, the answer to this question made me think we could get away with this.

@ghost ghost added the automerge label Oct 20, 2020
@mergify mergify bot merged commit 7f679b9 into master Oct 20, 2020
@mergify mergify bot deleted the samir/libs-scala/resources/customizable-contexts branch October 20, 2020 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aherrmann-da aherrmann-da Awaiting requested review from aherrmann-da

@cocreature cocreature Awaiting requested review from cocreature

@dasormeter dasormeter Awaiting requested review from dasormeter

@fabiotudone-da fabiotudone-da Awaiting requested review from fabiotudone-da

@gerolf-da gerolf-da Awaiting requested review from gerolf-da

@hurryabit hurryabit Awaiting requested review from hurryabit

@miklos-da miklos-da Awaiting requested review from miklos-da

@mziolekda mziolekda Awaiting requested review from mziolekda

@rautenrieth-da rautenrieth-da Awaiting requested review from rautenrieth-da

@S11001001 S11001001 Awaiting requested review from S11001001

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stefanobaghino-da @SamirTalwar