Skip to content

Conversation

@uranusjr
Copy link
Member

@uranusjr uranusjr commented Dec 20, 2021

Extending #20165 (for task #19625) because it turns out tracking just log_filename_template is not enough.

The LogFilename model is renamed to LogTemplate, and now contains two template fields filename (for log_filename_template) and task_prefix (for task_log_prefix_template). The two fields are tracked together when Airflow launches as previously.

The custom task log formatter is updated to use the database entry instead of the configuration, similar to the log filename renderer.

Since the previous change has not been released, I opted to update the migration in-place instead of creating a new one.

Also added some more tests on task_log_prefix_template.

@uranusjr uranusjr force-pushed the task-log-prefix-format-model branch 2 times, most recently from cd855d0 to 7230fa5 Compare December 20, 2021 21:34
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 20, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

The LogFilename model is renamed to LogTemplate, and now contains two
template fields 'filename' (for log_filename_template) and 'task_prefix'
(for task_log_prefix_template). The two fields are tracked together when
Airflow launches as previously.

The custom task log formatter is updated to use the database entry
instead of the configuration, similar to the log filename renderer.
@uranusjr uranusjr force-pushed the task-log-prefix-format-model branch from 5a0e195 to 5e4ec14 Compare December 21, 2021 01:01
Copy link
Member

@potiuk potiuk Dec 21, 2021

Choose a reason for hiding this comment

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

Should we turn it into a wanrning and fall-back to the first log template id ?

I think exception on missing log id is a bit too much as it might result in "stack-trace error" (maybe that is intended though?).

Also maybe the message could be somehow more specific. As the current one is not really actionable. I presume at the first start of airlfow there will be at least one entry created, so I guess the only case it can happen is when log_template_id is set in DagRun but missing in the LogTemplate.

Maybe we should be very explicit "Your dag_run table is corrupted. The entry in your dag_run table with dag_run_id: contains invalid log_template_id: <log_template_id>. You should set log_template_id for this field to one of the existing log_template_ids or set to NULL." . Or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can never happen because 1. None has special meaning, and 2. there’s a foreign key constraint so the database guanratees any non-null value points to something. Things are going very wrong if we reach this code, so I think it’s better to fail extremely loudly.

@uranusjr uranusjr force-pushed the task-log-prefix-format-model branch 3 times, most recently from e7f2d4e to faa865f Compare December 21, 2021 10:26
@uranusjr
Copy link
Member Author

tests/executors/test_dask_executor.py::TestDaskExecutor::test_dask_executor_functions:
ValueError: The futures should have finished; there is probably an error communicating with the Dask cluster.

Hmm. I don’t recall seeing this being flaky previously, maybe something new going wrong? Likely not related though.

@uranusjr uranusjr merged commit 302efad into apache:main Dec 21, 2021
@uranusjr uranusjr deleted the task-log-prefix-format-model branch December 21, 2021 12:26
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:logging area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) 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.

5 participants