[AIRFLOW-1602] LoggingMixin in DAG class#2602
Conversation
Within the DAG class we want to use the LoggingMixer for more transparent logger instead of creating a new anonymous logger
|
@Fokko, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mistercrunch, @bolkedebruin and @aoen to be potential reviewers. |
| orm_dag = DagModel(dag_id=dag.dag_id) | ||
| log = LoggingMixin().logger | ||
| log.info("Creating ORM DAG for %s", dag.dag_id) | ||
| dag.logger.info("Creating ORM DAG for %s", dag.dag_id) |
There was a problem hiding this comment.
This is odd. You are logging in a different class here. I kind of assume a log is private to the class (although we are not explicitly doing that)
There was a problem hiding this comment.
Not the subject of this PR, but this being a static method, where the first argument is an instance of the class is very odd.
There are two uses. One in utils/db.py:
for dag in dagbag.dags.values():
models.DAG.sync_to_db(dag, dag.owner, now)
This would make more sense as dag.sync_to_db(synctime=now). The other use is inside this method itself:
for subdag in dag.subdags:
DAG.sync_to_db(subdag, owner, sync_time, session=session)
which could be become subdag.sync_to_db(owner=self.owner, sync_time=sync_time, session=session)
Think that's worth adding as a standalone PR and not changing this logger use?
There was a problem hiding this comment.
(or not changing this logger use now, but in that PR)
There was a problem hiding this comment.
@ashb absolutely, that makes sense. A PR would be very welcome.
Codecov Report
@@ Coverage Diff @@
## master #2602 +/- ##
=========================================
- Coverage 70.71% 70.7% -0.02%
=========================================
Files 150 150
Lines 11566 11564 -2
=========================================
- Hits 8179 8176 -3
- Misses 3387 3388 +1
Continue to review full report at Codecov.
|
|
LGTM, given the follow up PRs |
5 participants
Dear Airflow maintainers,
As @ashb mentioned. Within the DAG class we want to use the LoggingMixer for more transparent logger instead of creating a new anonymous logger.
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
JIRA
Description
Tests
Commits