Skip to content

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 including HA (high availability) scheduler 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:CLI area:Scheduler including HA (high availability) scheduler area:Triggerer type:bug-fix Changelog: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple Triggerer processes keeps reassigning triggers to each other when job_heartbeat_sec is higher than 30 seconds.

4 participants