Skip to content

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

Merged
hussein-awala merged 1 commit into
apache:mainfrom
hussein-awala:fix/triggerers_heartbeats
Jun 30, 2023
Merged

hussein-awala merged 1 commit into
apache:mainfrom
hussein-awala:fix/triggerers_heartbeats

Conversation

@hussein-awala

Copy link
Copy Markdown
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>
@hussein-awala

Copy link
Copy Markdown
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

potiuk commented Jun 25, 2023

Copy link
Copy Markdown
Member

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

Comment thread airflow/models/trigger.py Resolved
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

potiuk commented Jun 29, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
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

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