[dagster-aws] Pipes AWS Glue Dagster run interruption handler

[danielgafni]

Summary & Motivation

I decided it was a good idea to add an automatic Glue job cleanup handler as there is a change for Spark jobs to spend a lot of unnecessary resources otherwise.

Had to rewrite the fake Glue clients to implement non-blocking job execution via subprocess.Popen

Strong Inception vibes in this PR

How I Tested These Changes

• Added a test with subprocess interruption. The test runs Dagster's materialize inside a multiprocessing.Process. Inside this process, the fake Glue client runs the Glue job in a subprocess.Popen. Once the Dagster process receives termination signal, the PipesGlueClient invokes the fake glue client to terminate the subprocess.Popen "job". We can register this call in the fake client and test for it.

[danielgafni]

• pipes-glue-interruption-tests-wip #23502
• [dagster-aws] Pipes AWS Glue Dagster run interruption handler #23354 👈
• [dagster-aws] add Pipes cloudwatch message reader #23353 : 2 other dependent PRs (#23440 , #23441 )
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @danielgafni and the rest of your teammates on Graphite

[danielgafni]

@schrockn should I be using try: ... except DagsterExecutionInterruptedError: ... instead of the current approach? I found it in other places in the codebase after submitting this PR.

[schrockn]

Adding @alangenfeld as he is generally more up-to-speed on this type of issues. Should we be terminating external processes launched by a run if that run is aborted?

[alangenfeld]

should I be using try: ... except DagsterExecutionInterruptedError: ... instead of the current approach?

yea i think its a bit easier to reason about and you don't need to worry about removing the handler

Should we be terminating external processes launched by a run if that run is aborted?

The feedback we got when we initially did not clean them up is that we should since they are orphaned and unreported if they continue. We do make the behavior optional in the subprocess pipes client.

you can reference https://github.com/dagster-io/dagster/pull/18685/files for some inspiration for how to put this under test

[schrockn]

Managing my queue. @danielgafni let me know when you have incorporate alex's feedback and want me to look again.

[danielgafni]

Hey @alangenfeld @schrockn, I added a test for Glue run interruption

[schrockn]

congrats for introducing only the third instance of the walrus to the codebase! I'm so trained not to use new features given how we have to support old Python version I totally forget they exist.

[schrockn]

Code seems fine to me. Please get the nod from @alangenfeld before merging as he is more conversant with these issues than I.

[alangenfeld]

nod