Page MenuHomeElementl

Bug: log k8s executor events
ClosedPublic

Authored by johann on Jul 7 2021, 5:44 PM.

Details

Summary

The executor events weren't being stored

Test Plan

integration

Diff Detail

Repository
R1 dagster
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

johann published this revision for review.Jul 7 2021, 6:02 PM
johann edited the summary of this revision. (Show Details)
johann added reviewers: alangenfeld, rexledesma.
alangenfeld added inline comments.
python_modules/libraries/dagster-k8s/dagster_k8s/executor.py
160–163

*

184–185

maybe add a comment here about manually created events needing to be sent handled like this

could also add a DagsterEvent.create method or something that did what the others static methods do

This revision is now accepted and ready to land.Jul 7 2021, 7:18 PM
johann requested review of this revision.Jul 7 2021, 9:25 PM

Changed approach to log from the step_delegating_executor rather than the step handlers, because the executor has the step contexts and can log correctly. this also means you don't have to remember to log the individual events.

concern with this approach is accidentally getting dupe events if anyone returns an event that was already logged (ie it was created with one of the static constructors that logs)

maybe just add a test like ** that checks for dupes?

integration_tests/test_suites/k8s-integration-test-suite/test_executor.py
92

**

python_modules/libraries/dagster-k8s/dagster_k8s/executor.py
184–195

tangential: should this not have some sort of message to indicate that the failure is due to the k8s job failing?

This revision is now accepted and ready to land.Jul 7 2021, 9:58 PM
This revision was automatically updated to reflect the committed changes.