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

feat(trigger): reload trigger if file changes #32032

Closed
wants to merge 1 commit into from

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Jun 20, 2023


^ 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.

@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label Jun 20, 2023
@Lee-W
Copy link
Member Author

Lee-W commented Jun 20, 2023

Hi @uranusjr, I recall you mentioning that reloading is generally not a good idea. However, @pauloventurab faced a syncing issue with the triggerer on #31743. I'm not sure if there're other better solutions than reloading the triggerer. I have drafted an idea of reloading only when necessary in this draft PR. I would appreciate your feedback on it. Thank you.

@uranusjr
Copy link
Member

I am very against this as well. This seems to solve the immediate problem, but will very likely to cause other issues down the road.

Why not just restart the triggerer process? The process is designed to be robust, and reloading it entirely is a fully supported operation.

@Lee-W
Copy link
Member Author

Lee-W commented Jun 21, 2023

I am very against this as well. This seems to solve the immediate problem, but will very likely to cause other issues down the road.

Yep, I guess that would be the case as well. That's why I'd like to have some feedback before investing more time crafting it 🙂 Thanks for the prompt feedback.

Why not just restart the triggerer process? The process is designed to be robust, and reloading it entirely is a fully supported operation.

If that's the case, should we comment on this solution on #31743, mark the issue as won't fix, and close it?

@uranusjr
Copy link
Member

I’m thinking maybe (not sure) we can still do something to reduce the confusion. Let’s continue in the issue thread instead.

@Lee-W
Copy link
Member Author

Lee-W commented Jun 21, 2023

Got it. Sounds good 👍 I'll check what can be done to reduce such confusion. Thanks!

@Lee-W Lee-W closed this Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler including HA (high availability) scheduler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants