Skip to content

Migrate DagFileProcessorManager._deactivate_stale_dags to Internal API #28476

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.

Sign up for GitHub

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

Jump to bottom
Merged
potiuk merged 9 commits into from
Jan 25, 2023
Merged

Migrate DagFileProcessorManager._deactivate_stale_dags to Internal API #28476

potiuk merged 9 commits into from
Jan 25, 2023

Conversation

@vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Dec 19, 2022

Migrate DagFileProcessorManager._deactivate_stale_dags to Internal API. Please while reviewing it to pay double attention to how I handle the logs. When discussing with @mhenc, we decided:

  • If a function is NOT executed though internal API, the logger should stay as is
  • If a function is executed through internal API, the logger should be separate

Very happy to hear your feedback and/or suggestions since, to be very honest, I am not a big fan of passing the logger as parameter

Closes #28270

@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label Dec 19, 2022
@vincbeck
Copy link
Contributor Author

vincbeck commented Dec 19, 2022

cc @potiuk , @mhenc

@vincbeck vincbeck force-pushed the vincbeck/_deactivate_stale_dags branch from d6acb64 to f10c9a2 Compare December 19, 2022 21:06
mhenc
mhenc reviewed Dec 20, 2022
View reviewed changes
Copy link
Contributor

@mhenc mhenc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just two small nits. Thanks!

airflow/api_internal/endpoints/rpc_api_endpoint.py Outdated
output = handler(**params)
output = handler(
**params,
log=logging.getLogger(f"airflow.internal_api.{method_name}"),
Copy link
Contributor

@mhenc mhenc Dec 20, 2022

Choose a reason for hiding this comment

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

Does it work if the method doesn't have "log" parameter?

Copy link
Contributor Author

@vincbeck vincbeck Dec 20, 2022

Choose a reason for hiding this comment

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

Oh! That's a good point! I guess not. I dont think we should handle logs this way then. The simplest way would be to create a logger log = logging.getLogger(__name__) in airflow/dag_processing/manager.py and use this logger in static methods. The only downside is the logger would be the same whether or not internal API in turn on

Copy link
Contributor

@mhenc mhenc Dec 21, 2022

Choose a reason for hiding this comment

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

I hoped that we can detect that the method has "log" argument and add it then.

If it's not possible to detect then we can send addition parameter from client(like "inject_log=true") and when seen on server-side then add "log" to params

Copy link
Contributor Author

@vincbeck vincbeck Dec 22, 2022

Choose a reason for hiding this comment

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

I implemented a solution which detects if the parameter "log" exists. If it does, then provide one specific to internal API

Copy link
Contributor

@mhenc mhenc Dec 27, 2022

Choose a reason for hiding this comment

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

This looks great! Thanks!

@vincbeck vincbeck mentioned this pull request Jan 5, 2023
Merged
@potiuk
Copy link
Member

potiuk commented Jan 20, 2023 •
edited
Loading

I guess that one should be updated after discussion in #28502 with @classmethod ?

@vincbeck
Copy link
Contributor Author

vincbeck commented Jan 20, 2023 •
edited
Loading

I guess that one should be updated after discussion in #28502 with @classmethod ?

Correct! I rather wait for #28502 to be merged and then I'll update this PR

@potiuk
Copy link
Member

potiuk commented Jan 20, 2023

I guess that one should be updated after discussion in #28502 with @classmethod ?

Correct! I rather wait for #28502 to be merged and then I'll update this PR

+1

@vincbeck
Copy link
Contributor Author

vincbeck commented Jan 25, 2023

PR updated :)

@potiuk potiuk merged commit 29a26a8 into apache:main Jan 25, 2023
@vincbeck vincbeck deleted the vincbeck/_deactivate_stale_dags branch January 26, 2023 15:11
@pierrejeambrun pierrejeambrun added this to the Airflow 2.6.0 milestone Feb 27, 2023
@pierrejeambrun pierrejeambrun added AIP-44 Airflow Internal API changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) labels Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk approved these changes

@jedcunningham jedcunningham Awaiting requested review from jedcunningham

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy

+1 more reviewer

@mhenc mhenc mhenc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AIP-44 Airflow Internal API area:Scheduler including HA (high availability) scheduler changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Milestone

Airflow 2.6.0

Development

Successfully merging this pull request may close these issues.

AIP-44 Migrate DagFileProcessorManager._deactivate_stale_dags to Internal API

4 participants

@vincbeck @potiuk @mhenc @pierrejeambrun