Skip to content

Refactor and extract shared request handler logic from supervisor _handle_request methods#65624

Merged
vincbeck merged 18 commits into
apache:mainfrom
leeyspaul:refactor/extract-high-overlap-request-handlers
May 21, 2026
Merged

Refactor and extract shared request handler logic from supervisor _handle_request methods#65624
vincbeck merged 18 commits into
apache:mainfrom
leeyspaul:refactor/extract-high-overlap-request-handlers

Conversation

@leeyspaul
Copy link
Copy Markdown
Contributor

@leeyspaul leeyspaul commented Apr 21, 2026

Extract shared request handling into request_handlers.py and reuse it in the task supervisor, triggerer, and DAG file processor.

This reduces duplicated _handle_request() logic across the subprocess supervisors and aligns GetXCom handling on the same response-normalization path.

Related: #65570


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below

Generated-by: Codex, GPT-5.4

Note:

No new tests here because this was intended as a behavior-preserving refactor and the affected handler paths already have existing supervisor/processor coverage.


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@leeyspaul
Copy link
Copy Markdown
Contributor Author

This branch is currently based on #65269 for the shared request_handlers.py structure. Once that PR merges, I’ll rebase this branch onto main.

@leeyspaul leeyspaul changed the title Refactor and extract shared request handler logic from supervisor _handle_request methods WIP: Refactor and extract shared request handler logic from supervisor _handle_request methods Apr 21, 2026
Comment thread task-sdk/src/airflow/sdk/execution_time/request_handlers.py
Comment thread task-sdk/src/airflow/sdk/execution_time/request_handlers.py
@leeyspaul leeyspaul changed the title WIP: Refactor and extract shared request handler logic from supervisor _handle_request methods Refactor and extract shared request handler logic from supervisor _handle_request methods Apr 21, 2026
@leeyspaul
Copy link
Copy Markdown
Contributor Author

This PR is currently waiting on #65269 to merge, and then we'll mark this as ready afterwards.

@ferruzzi two things below that I'd like to ask for your attention on!

  1. Please see the comment about the self.id for GetPrevSuccessfulDagRun here: Refactor and extract shared request handler logic from supervisor _handle_request methods #65624 (comment)
  2. I left the GetConnection and GetVariable alone in DFP as there was not an exact match with the helper (added masking secrets), I do agree that I think it should be included and probably changed. I figure if that's the case it might be another good first issue to contribute back to the community :).

@ferruzzi
Copy link
Copy Markdown
Contributor

Personally I think DFP should have masked GetConnection and GetVariable and I would personally call this a bugfix (and a security violation fix...) but maybe message #contributors in Slack to see if there are any obvious reasons not to?

@leeyspaul
Copy link
Copy Markdown
Contributor Author

leeyspaul commented Apr 22, 2026

Personally I think DFP should have masked GetConnection and GetVariable and I would personally call this a bugfix (and a security violation fix...) but maybe message #contributors in Slack to see if there are any obvious reasons not to?

Will do, I'll ask in Slack. Thank you.

Slack thread here: https://apache-airflow.slack.com/archives/C06K9Q5G2UA/p1776817558532799

@ferruzzi
Copy link
Copy Markdown
Contributor

Had a chat with @o-nikolas and @vincbeck this morning, their recommendation is to add the mask, but do it in its own PR so we can backport that one.

@leeyspaul
Copy link
Copy Markdown
Contributor Author

Had a chat with @o-nikolas and @vincbeck this morning, their recommendation is to add the mask, but do it in its own PR so we can backport that one.

That sounds good, I'll open up a separate PR for that one.

@leeyspaul
Copy link
Copy Markdown
Contributor Author

Whoops, sorry, still pending merge of #65269. Converted back to draft.

@leeyspaul leeyspaul force-pushed the refactor/extract-high-overlap-request-handlers branch from 059c5e9 to 32d2ba2 Compare April 24, 2026 18:20
@ferruzzi
Copy link
Copy Markdown
Contributor

#65269 is merged and I think this looks good. We should be ready to merge it, I think??

@leeyspaul leeyspaul force-pushed the refactor/extract-high-overlap-request-handlers branch from 9b22ef1 to f9e572b Compare May 18, 2026 01:41
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label May 18, 2026
@vincbeck vincbeck merged commit 075937c into apache:main May 21, 2026
113 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:DAG-processing area:Executors-core LocalExecutor & SequentialExecutor area:task-sdk area:Triggerer ready for maintainer review Set after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants