sjyangkevin commented on PR #68195: URL: https://github.com/apache/airflow/pull/68195#issuecomment-4775238311
Hi @ferruzzi , I did some analysis and trying to understand better the root cause for the test failure and also to understand better the implementation. The current implementation in the PR updates the `schema.json` to let it accept a list of strings, to make it "pass" the check. In short, the root cause is that the way deadline is persisted in serialized_dag getting updated but the test seems to validate on the previous format :thinking: . I think I agree with you that this change does hide these details in the process, and I am leaning towards to the solution you proposed to update `write_dag` and make it to work on a deepcopy of the data instead of modifying in place. Let me know if you think it is still a good direction to go with. Thanks! The blob in `serialized_dag.data` goes through two transformations. In the below code snippet, `deadline` is serialized into `dict` and `serialized_dag["deadline"]` is a list of dicts. https://github.com/apache/airflow/blob/ab4c0978a76e4f44148fccd3a4f4fa28404ca317/airflow-core/src/airflow/serialization/serialized_objects.py#L1704-L1706 The `SerializedDagModel.write_dag` method serialize a DAG and writes it into database. `_generate_deadline_uuids` is called. The dicts are lifted out into the `deadline_alert` table, and the blob is mutated in place to hold an array of UUID strings that reference those rows. https://github.com/apache/airflow/blob/ab4c0978a76e4f44148fccd3a4f4fa28404ca317/airflow-core/src/airflow/models/serialized_dag.py#L677-L682 When the `test_write_dag` in `airflow-core/tests/unit/models/test_serialized_dag.py::TestSerializedDagModel` run, it validates the persisted data, which is a list of UUID strings, with the shape of data defined in the `schema.json`. ``` result = session.scalar(select(SDM).where(SDM.dag_id == dag.dag_id)) DagSerialization.validate_schema(result.data) ``` However, the schema (schema.json:196-205) only allows three shapes: https://github.com/apache/airflow/blob/918124cb4171ecf739122365db4dee00a3d7a9f8/airflow-core/src/airflow/serialization/schema.json#L196-L205 The list of UUID strings doesn't match with any of the shapes, so raised the validation error. I also attempted to analyze and understand the implementation, below are some findings, but feel free to correct me if I am wrong. #58248 #61702 1. It promotes deadline alerts into its own tables, to better support UI for query, sort, etc. 2. At runtime the scheduler must point from a materialized deadline back to the alert definition that produced it. Each alert gets a UUID primary key. The `deadline` table carries a `deadline_alert_id` foreign key to it, and the Dag blob keeps the same UUID as its reference, so both the runtime row and the Dag point at one shared identity. 3. Re-parsing an unchanged Dag should not produce a new serialized version. Replacing definitions with UUIDs lets `write_dag` control what enters the hash. When a definition is unchanged it reuses the existing UUID, and for a name-only change it updates the table row while keeping the UUID, so the hash stays constant and no new version is created. -- 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]
