Page MenuHomePhabricator
Differential D5119

grpc ExecuteRun api call fixes
ClosedPublic
Actions

Authored by dgibson on Nov 12 2020, 7:48 PM.

Details

Reviewers
alangenfeld
max
prha
johann
Commits
R1:9727252f739b: grpc ExecuteRun api call fixes
Summary
  • Missing test coverage on the 'launch after cleaning up server case'
  • wait for the process to clean up before leaving the ExecuteRun API call
Test Plan

BK + Azure

Diff Detail

Repository
R1 dagster
Branch
grpccomputelogssadtimes (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

dgibson created this revision.Nov 12 2020, 7:48 PM
Harbormaster completed remote builds in B21084: Diff 25570.Nov 12 2020, 8:01 PM
dgibson requested review of this revision.Nov 12 2020, 8:01 PM
alangenfeld added a comment.Nov 12 2020, 8:53 PM

what still uses ExecuteRun ? I thought things used StartRun now

alangenfeld added inline comments.Nov 12 2020, 8:55 PM
python_modules/dagster/dagster/serdes/ipc.py
61–63 ↗(On Diff #25570)

nit: looking forward towards type hinting, is it worth making this Noneable instead of updating the callsite

alangenfeld requested changes to this revision.Nov 12 2020, 8:57 PM

should we just get rid of ExecuteRun in favor of StartRun ? - just request review if I missed the callsite thats still using it

This revision now requires changes to proceed.Nov 12 2020, 8:57 PM
dgibson updated this revision to Diff 25588.Nov 12 2020, 9:51 PM

remove altogether

dgibson updated this revision to Diff 25589.Nov 12 2020, 9:52 PM

up

Harbormaster failed remote builds in B21097: Diff 25588!Nov 12 2020, 10:04 PM
Harbormaster failed remote builds in B21098: Diff 25589!Nov 12 2020, 10:07 PM
dgibson updated this revision to Diff 25590.Nov 12 2020, 10:07 PM

missed a spot

Harbormaster completed remote builds in B21099: Diff 25590.Nov 12 2020, 10:20 PM
alangenfeld accepted this revision.Nov 12 2020, 10:28 PM

hold off til after release to land just to be safe but im pretty sure this is good

This revision is now accepted and ready to land.Nov 12 2020, 10:28 PM
Closed by commit R1:9727252f739b: grpc ExecuteRun api call fixes (authored by dgibson). · Explain WhyNov 13 2020, 1:30 AM
This revision was automatically updated to reflect the committed changes.
dgibson added a commit: R1:9727252f739b: grpc ExecuteRun api call fixes.

Revision Contents
Changeset List

PathSize
python_modules/
dagster/
dagster/
api/
grpc/
__generated__/
13 lines
protos/
9 lines
97 lines
dagster_tests/
api_tests/

Diff 25589

View Options

python_modules/dagster/dagster/api/execute_run.py

Loading...
View Options

python_modules/dagster/dagster/grpc/__generated__/api_pb2.py

Loading...
View Options

python_modules/dagster/dagster/grpc/__generated__/api_pb2_grpc.py

Loading...
View Options

python_modules/dagster/dagster/grpc/impl.py

Loading...
View Options

python_modules/dagster/dagster/grpc/protos/api.proto

Loading...
View Options

python_modules/dagster/dagster/grpc/server.py

Loading...
View Options

python_modules/dagster/dagster_tests/api_tests/test_api_execute_run.py

Loading...