[AIRFLOW-5944] Display rendered template_fields without accessing DAG files #6788

kaxil · 2019-12-11T10:32:44Z

One of the limitations with the current implementation of DAG serialization is that Templated fields still need access to DAG Files.

Potential solution 1: Store rendered version in the DB somewhere.
Limitation: Tasks that are not yet run won't be able to render the templated fields (this is a useful debugging aid, especially for those without CLI access to run airflow tasks render) .

Potential solution 2: Store both rendered & unrendered version in the DB somewhere. This will allow getting past the Limitation in Solution (1). We should store only the current unrendered version and store the last X number of rendered versions. This will also have an advantage that old task would have the rendered version that was correct at that time (which is not possible currently).
Limitation/Issue: Maintaining two separate tables without much benefit

Potential solution 3: Store rendered version in TI table and unrendered version in a separate column in DAG serialization table. This means that each TI would have an associated rendered field without creating an extra table and duplicating dag_id, task_id & execution_date. Once we start storing different version of serialized DAGs we can have each DAG row have it's unrendered version.

Issue link: AIRFLOW-5944

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID^*
Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

^* For document-only changes commit message can start with [AIRFLOW-XXXX].

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

ashb · 2019-12-11T10:36:25Z

airflow/migrations/versions/852ae6c715af_add_templated_fields_rendered_table.py

+    )
+
+    if conn.dialect.name == "mysql":
+        conn.execute("SET time_zone = '+00:00'")


We've got this in earlier migrations - I don't think we need it here too do we? Also: how about we just don't use a "TIMESTAMP" type, but "DATETIME" instead?

It won't contain the timezones though if we just use DATETIME! It might have undesirable effects when comparing current time with this time!

Really? https://www.eversql.com/mysql-datetime-vs-timestamp-column-types-which-one-i-should-use/ would imply otherwise.

(Also TIMESTAMP only supports dates up to 2038.)

From the same article :

To summarize, if you want to serve your date and time data the same way regardless of timezones, you can use the DATETIME type (which will also allow you to use all the date & type functions built in MySQL). Otherwise, you can use TIMESTAMP and serve the data on a per-timezone basis.

Also, from https://docs.sqlalchemy.org/en/13/core/type_basics.html#sqlalchemy.types.DateTime

For timezone support, use at least the TIMESTAMP datatype, if not the dialect-specific datatype object.

I read that and my conclusion is we should use DATETIME for mysql - Airflow puts in checks to ensure that we only ever store UTC datetimes in the DB.

By using a datetime column the migrations become a lot simpler.

airflow/models/templatedfields.py

airflow/migrations/versions/1d3a23a719bf_merge_heads.py

anitakar · 2020-01-29T12:49:19Z

Hey @kaxil. Do we have any ETA on when it will be merged?
And will it solve the problem of showing source code in web UI when webserver does not have access to dag folder?

mik-laj · 2020-01-29T12:53:37Z

I am also very interested in changes regarding DAG serialization. If you could share your work progress, I would be very grateful. WIP PR is better than no PR.

kaxil · 2020-01-29T15:22:33Z

Hi @anitakar @mik-laj - I am on holidays this week but would be fulltime on it from next week. Will keep you guys posted.

kaxil · 2020-02-03T11:55:14Z

2 tests are failing:

FAILED tests/operators/test_dagrun_operator.py::TestDagRunOperator::test_trigger_dagrun_operator_conf
FAILED tests/operators/test_dagrun_operator.py::TestDagRunOperator::test_trigger_dagrun_with_execution_date
= 2 failed, 4823 passed, 180 skipped, 7 xfailed, 1 xpassed, 130 warnings in 1623.07s (0:27:03) =

self = <_mysql.connection closed at 559364d105d0>
query = b"INSERT INTO rendered_task_instance_fields (dag_id, task_id, execution_date, rendered_fields) VALUES ('triggerdag', 'test', '2020-02-03 10:48:04.120893', '{}')"
    def query(self, query):
        # Since _mysql releases GIL while querying, we need immutable buffer.
        if isinstance(query, bytearray):
            query = bytes(query)
        if self.waiter is not None:
            self.send_query(query)
            self.waiter(self.fileno())
            self.read_query_result()
        else:
>           _mysql.connection.query(self, query)
E           sqlalchemy.exc.IntegrityError: (_mysql_exceptions.IntegrityError) (1062, "Duplicate entry 'triggerdag-test-2020-02-03 10:48:04' for key 'PRIMARY'")
E           [SQL: INSERT INTO rendered_task_instance_fields (dag_id, task_id, execution_date, rendered_fields) VALUES (%s, %s, %s, %s)]
E           [parameters: ('triggerdag', 'test', datetime.datetime(2020, 2, 3, 10, 48, 4, 120893, tzinfo=<Timezone [UTC]>), '{}')]
E           (Background on this error at: http://sqlalche.me/e/gkpj)

airflow/models/dagrun.py

airflow/models/templatedfields.py

ashb

Needs a test (with a json blob in the .py file) showing how this is stored.

Unless I'm blind you aren't reading RenderedTaskInstanceFields yet?

kaxil · 2020-02-03T12:58:58Z

Needs a test (with a json blob in the .py file) showing how this is stored.

Unless I'm blind you aren't reading RenderedTaskInstanceFields yet?

We are reading it here: https://github.com/apache/airflow/pull/6788/files#diff-948e87b4f8f644b3ad8c7950958df033R590-R602

airflow/www/views.py

…8051) This is because "The composite IN construct is not supported by all backends" Based on discussion in #6788 (comment) (cherry picked from commit 99370fe)

…(#8051) This is because "The composite IN construct is not supported by all backends" Based on discussion in apache/airflow#6788 (comment) (cherry picked from commit 99370fe770ad32f01cb9bcb38c97024dfe31ff51) GitOrigin-RevId: 5499302cc3c7fb49affe83e6ee08f8e40b11712c

…(#8051) This is because "The composite IN construct is not supported by all backends" Based on discussion in apache/airflow#6788 (comment) GitOrigin-RevId: 99370fe770ad32f01cb9bcb38c97024dfe31ff51

kaxil requested a review from ashb December 11, 2019 10:32

ashb reviewed Dec 11, 2019

View reviewed changes

airflow/models/templatedfields.py Outdated Show resolved Hide resolved

ashb reviewed Dec 11, 2019

View reviewed changes

airflow/models/templatedfields.py Outdated Show resolved Hide resolved

kaxil force-pushed the AIRFLOW-5944-rendering-templated-fields branch from ef008b5 to a61fc97 Compare December 12, 2019 00:35

github-actions bot added the area:webserver Webserver related Issues label Jan 5, 2020

kaxil mentioned this pull request Jan 20, 2020

[AIRFLOW-5946] Store source code in db #7217

Merged

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jan 26, 2020

kaxil added pinned Protect from Stalebot auto closing and removed stale Stale PRs per the .github/workflows/stale.yml policy file labels Jan 26, 2020

apache deleted a comment from stale bot Jan 26, 2020

kaxil force-pushed the AIRFLOW-5944-rendering-templated-fields branch from a61fc97 to aa27c4a Compare January 29, 2020 06:56

kaxil added the area:serialization label Jan 29, 2020

kaxil commented Jan 29, 2020

View reviewed changes

airflow/migrations/versions/1d3a23a719bf_merge_heads.py Outdated Show resolved Hide resolved

kaxil force-pushed the AIRFLOW-5944-rendering-templated-fields branch 4 times, most recently from 70be2e7 to 4639f07 Compare February 2, 2020 22:02