Page MenuHomeElementl

Bug: log k8s executor events
ClosedPublic

Authored by johann on Jul 7 2021, 5:44 PM.
Tags
None
Referenced Files
F2439160: D8739.id41162.diff
Tue, Aug 16, 7:18 PM
F2439159: D8739.id.diff
Tue, Aug 16, 7:18 PM
Unknown Object (File)
Mon, Aug 15, 8:04 AM
Unknown Object (File)
Fri, Aug 12, 10:50 AM
Unknown Object (File)
Fri, Aug 5, 3:14 PM
Unknown Object (File)
Wed, Aug 3, 6:40 AM
Unknown Object (File)
Sun, Jul 31, 11:24 PM
Unknown Object (File)
Jul 15 2022, 3:08 AM
Subscribers
None

Details

Summary

The executor events weren't being stored

Test Plan

integration

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

johann edited the summary of this revision. (Show Details)
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

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–197

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.