Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AIP 39: Documentation #17552

Merged
merged 21 commits into from
Sep 21, 2021
Merged

AIP 39: Documentation #17552

merged 21 commits into from
Sep 21, 2021

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Aug 11, 2021

Very much a work in progress. Ready for review now.

I have very low confidence on my ability writing comprehensive prose 🙂

@uranusjr uranusjr added the AIP-39 Timetables label Aug 11, 2021
docs/apache-airflow/dag-run.rst Outdated Show resolved Hide resolved
docs/apache-airflow/dag-run.rst Outdated Show resolved Hide resolved
docs/apache-airflow/concepts/dags.rst Outdated Show resolved Hide resolved
docs/apache-airflow/dag-run.rst Outdated Show resolved Hide resolved
docs/apache-airflow/dag-run.rst Outdated Show resolved Hide resolved
docs/apache-airflow/dag-run.rst Outdated Show resolved Hide resolved
docs/apache-airflow/dag-run.rst Outdated Show resolved Hide resolved
docs/apache-airflow/howto/timetable.rst Outdated Show resolved Hide resolved
docs/apache-airflow/howto/timetable.rst Outdated Show resolved Hide resolved
docs/apache-airflow/howto/timetable.rst Outdated Show resolved Hide resolved
docs/apache-airflow/howto/timetable.rst Outdated Show resolved Hide resolved
docs/apache-airflow/howto/timetable.rst Outdated Show resolved Hide resolved
@uranusjr uranusjr force-pushed the aip-39-docs branch 7 times, most recently from ef28489 to dd07a4b Compare August 17, 2021 04:12
@uranusjr
Copy link
Member Author

I created an example DAG showcasing how timetable subclassing works and finished the howto guide referencing the example DAG. I’m more or less done for now, but since the howto’s content depends on changes in #17414, this will stay a draft until that PR is merged (or at least close to it).

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really clear to me (but then I'm familiar with both the code and the proposal, so I'm not the best to judge.)

docs/apache-airflow/dag-run.rst Outdated Show resolved Hide resolved
docs/apache-airflow/howto/timetable.rst Outdated Show resolved Hide resolved
docs/apache-airflow/howto/timetable.rst Outdated Show resolved Hide resolved
docs/apache-airflow/howto/timetable.rst Outdated Show resolved Hide resolved
docs/apache-airflow/howto/timetable.rst Outdated Show resolved Hide resolved
docs/apache-airflow/howto/timetable.rst Outdated Show resolved Hide resolved
@uranusjr
Copy link
Member Author

Tests are failing because we haven’t actually implemented DAG(timetable=...) yet. Otherwise CI seems satisfied.

@uranusjr
Copy link
Member Author

I think this is ready for review. In addition to the new timetable how-to page, I also rewrote many execution date and execution_date occurrences to refer to the data intervals instead where it makes sense.

@uranusjr uranusjr marked this pull request as ready for review September 14, 2021 02:29
@kaxil
Copy link
Member

kaxil commented Sep 17, 2021

(Just rebased)

@kaxil
Copy link
Member

kaxil commented Sep 20, 2021

Might need another rebase

Not all usages can be removed, unfortunately, since there are still many
usages requiring the value, especially in Python APIs.

This also adds additional values for Sentry tags and Elastic log
filenames based on data interval instead of execution date, so the user
can migrate as soon as possible, even if we have not changed the default.
We need to use execution_date here :/
A DAG run CAN technically start before its data interval ends, but
that's an edge case we shouldn't cover early in the documentation, so
let's make wording vague to focus on the general cases to make things
less complicated for users reading this for the first time.
A custom timetable requires plugin initialization, which is not done for
unit tests. But we really don't want to automatically register that
plugin since it makes testing an unregistered plugin impossible. Since
that example DAG is very short and referenced only once anyway, let's
just not bother.
This ensures the log ID is unique across the board, similar to the
guarantee we had prior to AIP-39 (via execution date).
We don't want to expose TaskInstance(execution_date=...) anymore, but
the extra link interface still relies on execution date, so we need an
alternative.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-39 Timetables full tests needed We need to run full set of tests for this PR to merge kind:documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants