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

Fix triggerers alive check and add a new conf for triggerer heartbeat rate #32123

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

hussein-awala
Copy link
Member

closes: #32121

This PR:

  • adds a new configuration for the triggerer heartbeat rate
  • fixes the assign_unassigned method: thhe static 30-second threshold is replaced with a dynamically calculated threshold obtained by multiplying the triggerer heartbeat rate by a grace multiplier (2.1).

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

… rate

Signed-off-by: Hussein Awala <hussein@awala.fr>
@boring-cyborg boring-cyborg bot added area:CLI area:Scheduler Scheduler or dag parsing Issues labels Jun 25, 2023
@hussein-awala hussein-awala added this to the Airlfow 2.6.3 milestone Jun 25, 2023
@hussein-awala
Copy link
Member Author

@ephraimbuddy @pierrejeambrun I add a new Airflow configuration in this PR, but I can fix the issue without it where it fallback to scheduler heartbeat rate.

  1. should I create a separate PR for the new conf?
  2. should I make it fallback to scheduler heartbeat rate instead of its default (5) to avoid the breaking change?

WDYT?

@potiuk
Copy link
Member

potiuk commented Jun 25, 2023

I think it is fine to both: add new config and fallback to 5 seconds.

job_heartbeat_sec:
description: |
How often to heartbeat the Triggerer job to ensure it hasn't been killed.
version_added: 2.6.3
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are adding a new configuration, this should be moved to 2.7.0

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily. I think we do not have a rule "do not add new configuration options" in patchlevel. We have the rule of not adding new features, but new configuration might be added in order to implement a bugfix (which I think is the case here).

I think it's OK (and we've done that in the past) that we introduced new configuration in patchlevel version if they served bugfix purpose. There are a few configs already that were added in non- .0 versions (smtp ones for example).

Copy link
Member

Choose a reason for hiding this comment

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

(and yes, that was also my initial reaction, but I thought about it and I looked at the past configuration entries we created and it's not obvious that "new configuration == new feature".

Copy link
Member Author

Choose a reason for hiding this comment

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

@ephraimbuddy Do you agree with @potiuk, or should I move it to 2.7.0?

@potiuk
Copy link
Member

potiuk commented Jun 29, 2023

There were some "stalled" GA jobs running for that one I closed/reopened to rebuild.

I take the thumbs-up reaction of @ephraimbuddy as agremeent :). And I think this one is worthy cherry-picking to 2.6.3 :)

@hussein-awala
Copy link
Member Author

I take the thumbs-up reaction of @ephraimbuddy as agremeent :). And I think this one is worthy cherry-picking to 2.6.3 :)

Great! I asked him because of this thumbs-up.
I'll merge it.

@hussein-awala hussein-awala merged commit d117728 into apache:main Jun 30, 2023
76 of 82 checks passed
ephraimbuddy pushed a commit that referenced this pull request Jul 6, 2023
… rate (#32123)

Signed-off-by: Hussein Awala <hussein@awala.fr>
(cherry picked from commit d117728)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI area:Scheduler Scheduler or dag parsing Issues area:Triggerer type:bug-fix Changelog: Bug Fixes
Projects
None yet
4 participants