jason810496 commented on code in PR #72474:
URL: https://github.com/apache/airflow/pull/72474#discussion_r4079499863
##########
airflow-core/src/airflow/api/common/delete_dag.py:
##########
@@ -82,7 +82,10 @@ def delete_dag(dag_id: str, keep_records_in_log: bool =
True, *, session: Sessio
# This handles the case when the dag_id is changed in the file
session.execute(
delete(ParseImportError).where(
- ParseImportError.filename == dag.relative_fileloc,
+ or_(
+ ParseImportError.source_reference == dag.relative_fileloc,
+ ParseImportError.filename == dag.fileloc,
+ ),
Review Comment:
IIUC, should it be like the following statement? Since the
`ParseImportError.filename` still stores the relative path. Unless we want to
make the `filename` storing the asb path, then we should update the write path
in the same PR as well.
Not sure do I understand correctly or not.
```suggestion
or_(
ParseImportError.source_reference == dag.relative_fileloc,
ParseImportError.filename == dag.relative_fileloc,
),
```
##########
airflow-core/src/airflow/models/errors.py:
##########
@@ -34,12 +35,17 @@ class ParseImportError(Base):
id: Mapped[int] = mapped_column(Integer, primary_key=True)
timestamp: Mapped[datetime | None] = mapped_column(UtcDateTime,
nullable=True)
filename: Mapped[str | None] = mapped_column(String(1024), nullable=True)
+ source_reference: Mapped[str | None] = mapped_column(String(1024),
nullable=True)
Review Comment:
Would it be better to align with the DagModel length constriant?
https://github.com/apache/airflow/blob/b6538c32c8b1cdcb7b818e174a8b09d69ae18ad6/airflow-core/src/airflow/models/dag.py#L343
Thought the `ParseImportError.filename` should be fixed to 2000 as well, but
it's out of scope of current PR.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/dag_warning.py:
##########
@@ -30,7 +30,7 @@ class DAGWarningResponse(BaseModel):
"""Dag Warning serializer for responses."""
dag_id: str
- warning_type: DagWarningType
+ warning_type: DagWarningType | str
Review Comment:
How about updating the definition of the `DagWarningType` so that all the
downstream reference callers won't update all the annotation across modules.
And this also keep the `DagWarningType` as single source of truth to avoid the
further drift.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_sources.py:
##########
@@ -103,6 +103,7 @@ def get_dag_source(
content=content,
version_number=dag_version.version_number,
dag_display_name=dag_version.dag_model.dag_display_name,
+ language=dag_version.dag_code.language,
Review Comment:
Not necessary in this PR, but we need to track additional issue for the
early joineload to avoid additional round trip introduce in this change. Since
this will not only touch the API layer but also the model layer so it would be
better to have a dedicated PR for this follow-up change.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_sources.py:
##########
@@ -103,6 +103,7 @@ def get_dag_source(
content=content,
version_number=dag_version.version_number,
dag_display_name=dag_version.dag_model.dag_display_name,
+ language=dag_version.dag_code.language,
Review Comment:
Not necessary in this PR, but we need to track an additional issue for the
early joineload to avoid additional round trip introduce in this change. Since
this will not only touch the API layer but also the model layer so it would be
better to have a dedicated PR for this follow-up change.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
