You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dingo4dev (via GitHub)" <gi...@apache.org> on 2025/06/25 10:07:13 UTC
[PR] Fix `logical_date` handling for using `exeuction_date_fn` in `ExternalTaskSensor` [airflow]
dingo4dev opened a new pull request, #52237:
URL: https://github.com/apache/airflow/pull/52237
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
closes #52236
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `logical_date` handling for using `exeuction_date_fn` in `ExternalTaskSensor` [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #52237:
URL: https://github.com/apache/airflow/pull/52237#issuecomment-3032837364
I agree with Pavan here, as I mentioned in my comment. It is breaking for `_handle_execution_date_fn`, and it could potentially be used by users.
>IMO: The private method is intended for internal use only, and not expect external code to depend on these, though subclasses are an exception. But I couldn't find any subclass currently extend it. Please correct me if I am wrong.
Normally, I agree that they should be private and not used, but from a user perspective, if it works via calling a private method to solve a problem, there is nothing stopping users from using and solving the problem without creating a public one or a custom bespoke solution in their local/envs. For example, you are creating your sensor extended from ExternalTaskSensor like `CustomSensor(ExternalTaskSensor)`. In this case, those methods are not counted as private since it will be just `super()._handle_execution_date_fn`, just a method from the extended class, where no external call is made
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `logical_date` handling for using `exeuction_date_fn` in `ExternalTaskSensor` [airflow]
Posted by "dingo4dev (via GitHub)" <gi...@apache.org>.
dingo4dev closed pull request #52237: Fix `logical_date` handling for using `exeuction_date_fn` in `ExternalTaskSensor`
URL: https://github.com/apache/airflow/pull/52237
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `logical_date` handling for using `exeuction_date_fn` in `ExternalTaskSensor` [airflow]
Posted by "dingo4dev (via GitHub)" <gi...@apache.org>.
dingo4dev commented on PR #52237:
URL: https://github.com/apache/airflow/pull/52237#issuecomment-3028182447
@gopidesupavan Thanks for the review and for highlighting the concern. I agree it's a good point to refactor the functions for DRY.
IMO: The private method is intended for internal use only, and not expect external code to depend on these, though subclasses are an exception. But I couldn't find any subclass currently extend it.
I will close this PR if there is no any other suggestion/thought.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `logical_date` handling for using `exeuction_date_fn` in `ExternalTaskSensor` [airflow]
Posted by "dingo4dev (via GitHub)" <gi...@apache.org>.
dingo4dev commented on PR #52237:
URL: https://github.com/apache/airflow/pull/52237#issuecomment-3022237432
TBR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `logical_date` handling for using `exeuction_date_fn` in `ExternalTaskSensor` [airflow]
Posted by "gopidesupavan (via GitHub)" <gi...@apache.org>.
gopidesupavan commented on PR #52237:
URL: https://github.com/apache/airflow/pull/52237#issuecomment-3026990189
I would consider this will be breaking changing for `_handle_execution_date_fn` thought its private people might use this function directly in subclasses or call this function.
Probably worth refactoring `def _get_dttm_filter(self, context): ` according to https://github.com/apache/airflow/pull/52431/files#diff-cd0a81a3fe4e14f46d41d402eabcb273554245c7e676e549b8b84030a99d4870R525 , you can call `_get_logical_date` with context from `_get_dttm_filter`.
Feel free to workout.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `logical_date` handling for using `exeuction_date_fn` in `ExternalTaskSensor` [airflow]
Posted by "dingo4dev (via GitHub)" <gi...@apache.org>.
dingo4dev commented on PR #52237:
URL: https://github.com/apache/airflow/pull/52237#issuecomment-3067505478
Thank you @bugraoz93 and @gopidesupavan for comments and thoughts. Will close this PR due to potential breaking change for this function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `logical_date` handling for using `exeuction_date_fn` in `ExternalTaskSensor` [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #52237:
URL: https://github.com/apache/airflow/pull/52237#discussion_r2167035892
##########
providers/standard/src/airflow/providers/standard/sensors/external_task.py:
##########
@@ -529,7 +529,7 @@ def get_external_task_group_task_ids(self, session, dttm_filter):
dttm_filter, self.external_task_group_id, self.external_dag_id, session
)
- def _handle_execution_date_fn(self, context) -> Any:
+ def _handle_execution_date_fn(self, logical_date, context) -> Any:
Review Comment:
I think this method is purely for backwar compatibility and shouldn't have this breaking change here
cc: @kaxil what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `logical_date` handling for using `exeuction_date_fn` in `ExternalTaskSensor` [airflow]
Posted by "dingo4dev (via GitHub)" <gi...@apache.org>.
dingo4dev commented on code in PR #52237:
URL: https://github.com/apache/airflow/pull/52237#discussion_r2168025122
##########
providers/standard/src/airflow/providers/standard/sensors/external_task.py:
##########
@@ -529,7 +529,7 @@ def get_external_task_group_task_ids(self, session, dttm_filter):
dttm_filter, self.external_task_group_id, self.external_dag_id, session
)
- def _handle_execution_date_fn(self, context) -> Any:
+ def _handle_execution_date_fn(self, logical_date, context) -> Any:
Review Comment:
@bugraoz93 Thanks for review.
IMO, the backward compatibility is already handled in caller function, it is better to keep consistent and align with the caller.
https://github.com/apache/airflow/blob/3576530b02631fd8f735baf01cb4d33342875198/providers/standard/src/airflow/providers/standard/sensors/external_task.py#L261-L273
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org