Page MenuHomePhabricator

Split DagterGraphQLContext into ProcessContext and RequestContext
AcceptedPublic

Authored by sashank on Wed, Jan 13, 4:40 PM.

Details

Reviewers
alangenfeld
Summary

This diff splits the DagsterGraphQLContext into two separate classes:

  • ProcessContext
  • RequestContext

This split is only meaningful in the context of the Dagit webserver. The ProcessContext is long-lived, and the RequestContext is created for every request. The RequestContext stores (1) a reference to all the repository locations on the ProcessContext at the start of the request and (2) a snapshot of the Workspace state at the start of the request.

This is needed due to the introduction of the repository location watch threads that can modify the global context. The problem was that if a request was accessing a repository location at the same time the watch thread was cleaning it up, bad things would obviously happen.

Instead, the watch threads only run on the ProcessContext and are free to reload the repository locations and workspace as they like. RepositoryLocationHandles are not immediately cleaned up on location reload, and instead are cleaned up whenever all references to them in both the ProcessContext and all RequestContexts are gone.

This diff also brings back the sqlite_instance_deployed_grpc_env test suite.

Test Plan

unit

Diff Detail

Repository
R1 dagster
Branch
request-context-2
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Harbormaster returned this revision to the author for changes because remote builds failed.Wed, Jan 13, 10:57 PM
Harbormaster failed remote builds in B24329: Diff 29608!
  • Remove all contextmanagers and replace with del
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jan 14, 12:09 AM
Harbormaster failed remote builds in B24340: Diff 29622!
sashank retitled this revision from (1/n request-context-refactor) Split DagterGrqphQLContext into ProcessContext and RequestContext to Split DagterGrqphQLContext into ProcessContext and RequestContext.Thu, Jan 14, 12:26 AM
sashank edited the summary of this revision. (Show Details)
sashank added a reviewer: alangenfeld.
sashank edited the summary of this revision. (Show Details)

add tests

sashank retitled this revision from Split DagterGrqphQLContext into ProcessContext and RequestContext to Split DagterGraphQLContext into ProcessContext and RequestContext.Thu, Jan 14, 1:32 PM

zoom debugging / review

This revision now requires changes to proceed.Thu, Jan 14, 3:54 PM
  • Update subscription server

Coming up: Test to verify cleanup

nice

python_modules/dagit/dagit/app.py
55

+ comment

python_modules/dagster-graphql/dagster_graphql/implementation/context.py
17–37

+ comment

163

+ comment

python_modules/dagster-graphql/dagster_graphql/schema/roots.py
534–535

nit: could just return the repo location that was updated? ignore if theres a good reason why not

python_modules/dagster/dagster/cli/workspace/workspace.py
9–11

+ comment

to your queue for mentioned test and additional comments

This revision now requires changes to proceed.Thu, Jan 14, 5:28 PM
  • Add garbage collection tests
  • fix lint
python_modules/dagster-graphql/dagster_graphql/schema/roots.py
534–535

we need to recreate a request context because all the getters are only on the request context

does the watch thread need to get turned back on?

makeitso

This revision is now accepted and ready to land.Thu, Jan 14, 6:07 PM

does the watch thread need to get turned back on?

yup just need to rebase

  • rebase and turn on watch thread
  • bring back postgres deployed test suite