xinbinhuang commented on a change in pull request #15389:
URL: https://github.com/apache/airflow/pull/15389#discussion_r638420317
##########
File path: airflow/api_connexion/endpoints/task_instance_endpoint.py
##########
@@ -248,7 +248,7 @@ def post_clear_task_instances(dag_id: str, session=None):
error_message = f"Dag id {dag_id} not found"
raise NotFound(error_message)
reset_dag_runs = data.pop('reset_dag_runs')
- task_instances = dag.clear(get_tis=True, **data)
+ task_instances = dag.clear(get_ti_instances=True, **data)
Review comment:
Ah I see. I think these are some good reasons to move it to
`get_ti_instances`, but I would still prefer to keep `get_tis` and add the
`get_ti_keys`.
> because they are actually TaskInstance (model) instances.
- Though it makes sense, I think it's a bit unconventional as normally
people just reference the instances as lowercase to the model class name (i.e.
`class User(Base)` -> `user = User(...)` ) as similar to an instance(object) of
a class.
- Another concern from me is that people in the future may feel confused
about the naming when they come across this.
> different to the old name (so I could make sure I had caught all
references)
The `dag.clear()` method is a public API. Would this cause breaking changes?
Though I doubt people actually use `dag.clear()` in writing the DAG, the method
may still be used in testing. In this case, we still need to keep the old
keyword with deprecation warnings.
WDYT? I am also curious about other people's opinions.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
