Page MenuHomeElementl

Share grpc server processes between daemon threads
ClosedPublic

Authored by dgibson on Feb 28 2021, 4:58 AM.

Details

Summary

This creates a new abstraction that we can use to share information about how to find the gRPC server for a given origin.

Two things this would enable:

  • Only creating one managed gRPC process per daemon, even though they're in different thread
  • Paving the way for having the daemon manage gRPC server lifecycles in other ways besides just processes - for example, spinning up a docker container or k8s pod for each workspace entry, then saving that information to the registry where other daemons in other threads can know how to find it.

Still sorting out testing and fleshing out, but happy to hear any early thoughts.

Test Plan

BK, Integration

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 28 2021, 5:16 AM
Harbormaster failed remote builds in B26601: Diff 32505!
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 28 2021, 10:43 PM
Harbormaster failed remote builds in B26604: Diff 32508!
dgibson retitled this revision from Share grpc server processes between daemon threads (WIP) to WIP: Share grpc server processes between daemon threads.
dgibson edited the summary of this revision. (Show Details)
dgibson added reviewers: johann, alangenfeld, sashank, prha.

up

dgibson published this revision for review.Mar 1 2021, 3:33 PM

opening up for any initial feedback

remove the new handle, that's a bit much - augment GrpcRepositoryLocationHandle instead to include a heartbeat and custom port/socket/host

python_modules/dagster/dagster/core/host_representation/grpc_server_registry.py
16

nit: maybe 'Endpoint' instead of Status? Since location is taken

python_modules/dagster/dagster/core/host_representation/handle_manager.py
61

It's interesting that we have two layers of caching, the handle and the status. It does seem like it separates concerns well. A comment here would definitely help, to explain that the status in the existing handle timed out.

feedback, more tests, add a base class that we can use to create new registries (e.g. for docker containers)

dgibson retitled this revision from WIP: Share grpc server processes between daemon threads to Share grpc server processes between daemon threads.
dgibson edited the test plan for this revision. (Show Details)

run integration tests too

this looks pretty good to me... test_process_server_registry is great.

python_modules/dagster/dagster/core/host_representation/grpc_server_registry.py
49–52

maybe pull these out as constants (e.g. DEFAULT_CLEANUP_INTERVAL, DEFAULT_HEARTBEAT_INTERVAL)

python_modules/dagster/dagster_tests/core_tests/host_representation_tests/test_grpc_server_registry.py
72

nice

This revision is now accepted and ready to land.Mar 2 2021, 4:57 PM
python_modules/dagster/dagster_tests/core_tests/host_representation_tests/test_grpc_server_registry.py
38

nit: this test is real slow, not sure how easy that would be to fix. A notable stall while running the core tests locally