feat: Add optional static_url to BaseOperatorLink to skip XCom for constant links - #70343
feat: Add optional static_url to BaseOperatorLink to skip XCom for constant links#70343iRAFEEK wants to merge 7 commits into
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
aaron-y-chen
left a comment
There was a problem hiding this comment.
Hi, thanks for the PR! I wonder whether this would still write to XCom when static_url is set.
airflow/task-sdk/src/airflow/sdk/execution_time/task_runner.py
Lines 2261 to 2263 in 7217f84
It seems _xcom_push_to_db() is executed unconditionally after get_link(). Would you mind taking a look?
| or push anything to XCom during task execution. | ||
|
|
||
| :return: A static URL string, or ``None`` to use the XCom-based :meth:`get_link`. | ||
| """ |
There was a problem hiding this comment.
It might be a good idea to add this informatio to the relevant site doc too.
finalize() previously called _xcom_push_to_db() unconditionally for every operator extra link, so a link declared via static_url was still written to the metadata DB on each task run -- even though that value is never read back (the render path in SerializedBaseOperator.get_extra_links resolves the URL by calling get_link() directly). This defeated the point of static_url. Skip the XCom write when a link is static, add a task_runner test asserting no push happens for a static link (dynamic links unchanged), and document static_url in the define-extra-link howto. Addresses review feedback from @aaron-y-chen and @zach-overflow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: iRAFEEK <rafeekmagdy28@gmail.com>
|
@aaron-y-chen good catch, you're right. |
|
@zach-overflow good call, added a |
A newer ruff (0.16.0, pulled in via main) flags PT012 on the pytest.raises block in test_operatorlink.py (it contained a class definition plus the instantiation). Move the class definition out of the with-block so only the raising call remains inside; behaviour is unchanged (the TypeError is raised at instantiation). Also add missing trailing newlines to operatorlink.py and test_operatorlink.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: iRAFEEK <rafeekmagdy28@gmail.com>
|
Thanks again for the reviews @aaron-y-chen @zach-overflow! I've pushed:
Everything's green locally (including |
Closes #55432
Problem
BaseOperatorLink.get_link()was an@abstractmethod, forcing everysubclass to implement it even when the link URL is a constant (e.g.,
a documentation link). This meant operators with static links still had
to push to XCom during
execute()and read from the metadata databaseon every task run, adding unnecessary overhead.
Solution
This PR introduces an optional
static_urlproperty onBaseOperatorLink:static_urldefaults toNone(no behavior change for existing subclasses)get_link()is no longer@abstractmethod; its default implementationreturns
static_urlif set, otherwise raisesNotImplementedErrorwith a helpful message guiding the developer to override one or the other
static_urlinsteadof
get_link(), with zero XCom involvementBackward Compatibility
Fully backward-compatible. All existing subclasses that implement
get_link()continue to work without any changes.Example
Tests
Added
task-sdk/tests/task_sdk/bases/test_operatorlink.pycovering:static_urldefaults toNonestatic_urlreturns correct URL fromget_link()get_link()directly still works (existing behavior)NotImplementedErrorwith a clear messagenameremains abstract