Rename to_delete to to_cancel in TriggerRunner
#20658
Conversation
The queue's purpose is to track triggers that need to be canceled. I found the language `to_delete` a bit confusing because for one it does not actually delete them but cancel them. The deletion work is actually in `cleanup_finished_triggers`. It seems that the canceling of triggers with this method will usually not do anything and it's only for cancelling triggers that are currently running but for whatever reason no longer should be. E.g. when a task is killed and therefore the trigger is no longer needed, or some multi-triggerer scenarios.
boring-cyborg
bot
added
the
area:Scheduler
Just chatted with @ashb and his position is that methods like this on the trigger internals, or scheduler internals, should be considered private and can be refactored freely. But added that we should probablly also try to indicate that somewhere in the docs -- that there's no guarantee that they won't change from [minor] version to version. |
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
github-actions
bot
added
the
full tests needed
I think we should - rather than explaining this come up with the list of Classes/APIs (Protocols + Models ?) that would become our official Python API that we will guarantee to not change until major version increase. I don't think there will be awfully lot of those and I think in most cases it wil be enough to just list the classes that we treat this way. |
|
Yeah, my view is that the "public" API is, loosely
So Executors are part of the public API, but the SchedulerJob, nor TriggererJob are not. Clearly this isn't a fully description but the ground work of what I think it should be |
Yeah. Some (obvious) models Variables, Connections etc (they will become non-SQLAlchemy Models really when we agree/implement this part of the API in AIP-44) + configuration + some logging part + some base opareators (DBapi for example) . Those on top of what you wrote @ashb |
The queue's purpose is to track triggers that need to be canceled. The language `to_delete` was a bit confusing because for one it does not actually delete them but cancel them. The deletion work is actually in `cleanup_finished_triggers`. It seems that this method will usually not do anything and it's only for cancelling triggers that are currently running but for whatever reason no longer should be. E.g. when a task is killed and therefore the trigger is no longer needed, or some multi-triggerer scenarios. So putting cancel in the name also highlights that this is about stopping running triggers, not e.g. purging completed ones. (cherry picked from commit c20ad79)
The queue's purpose is to track triggers that need to be canceled. The language `to_delete` was a bit confusing because for one it does not actually delete them but cancel them. The deletion work is actually in `cleanup_finished_triggers`. It seems that this method will usually not do anything and it's only for cancelling triggers that are currently running but for whatever reason no longer should be. E.g. when a task is killed and therefore the trigger is no longer needed, or some multi-triggerer scenarios. So putting cancel in the name also highlights that this is about stopping running triggers, not e.g. purging completed ones. (cherry picked from commit c20ad79)
The queue's purpose is to track triggers that need to be canceled. The language `to_delete` was a bit confusing because for one it does not actually delete them but cancel them. The deletion work is actually in `cleanup_finished_triggers`. It seems that this method will usually not do anything and it's only for cancelling triggers that are currently running but for whatever reason no longer should be. E.g. when a task is killed and therefore the trigger is no longer needed, or some multi-triggerer scenarios. So putting cancel in the name also highlights that this is about stopping running triggers, not e.g. purging completed ones. (cherry picked from commit c20ad79)
The queue's purpose is to track triggers that need to be canceled. The language `to_delete` was a bit confusing because for one it does not actually delete them but cancel them. The deletion work is actually in `cleanup_finished_triggers`. It seems that this method will usually not do anything and it's only for cancelling triggers that are currently running but for whatever reason no longer should be. E.g. when a task is killed and therefore the trigger is no longer needed, or some multi-triggerer scenarios. So putting cancel in the name also highlights that this is about stopping running triggers, not e.g. purging completed ones. (cherry picked from commit c20ad79)
The queue's purpose is to track triggers that need to be canceled. The language `to_delete` was a bit confusing because for one it does not actually delete them but cancel them. The deletion work is actually in `cleanup_finished_triggers`. It seems that this method will usually not do anything and it's only for cancelling triggers that are currently running but for whatever reason no longer should be. E.g. when a task is killed and therefore the trigger is no longer needed, or some multi-triggerer scenarios. So putting cancel in the name also highlights that this is about stopping running triggers, not e.g. purging completed ones. (cherry picked from commit c20ad79)
The queue's purpose is to track triggers that need to be canceled. The language `to_delete` was a bit confusing because for one it does not actually delete them but cancel them. The deletion work is actually in `cleanup_finished_triggers`. It seems that this method will usually not do anything and it's only for cancelling triggers that are currently running but for whatever reason no longer should be. E.g. when a task is killed and therefore the trigger is no longer needed, or some multi-triggerer scenarios. So putting cancel in the name also highlights that this is about stopping running triggers, not e.g. purging completed ones. (cherry picked from commit c20ad79)
ephraimbuddy
added
type:misc/internal
The queue's purpose is to track triggers that need to be canceled. The language `to_delete` was a bit confusing because for one it does not actually delete them but cancel them. The deletion work is actually in `cleanup_finished_triggers`. It seems that this method will usually not do anything and it's only for cancelling triggers that are currently running but for whatever reason no longer should be. E.g. when a task is killed and therefore the trigger is no longer needed, or some multi-triggerer scenarios. So putting cancel in the name also highlights that this is about stopping running triggers, not e.g. purging completed ones. (cherry picked from commit c20ad79)
The queue's purpose is to track triggers that need to be canceled. I found the language
to_deletea bit confusing because for one it does not actually delete them but cancel them. The deletion work is actually incleanup_finished_triggers(and there it's deleting only from thetriggersdict and not the database. Deletion from the database happens elsewhere.It seems that this method will usually not do anything. The normal flow is that the trigger process will exit normally and be purged from the
triggersdict by the cleanup process. This method is more for exceptional circumstances where we need to cancel (in other words terminate) triggers that are currently running but for whatever reason no longer should be. E.g. when a task is killed and therefore the trigger is no longer needed, or some multi-triggerer scenarios.Additionally the use of the word deletion here has an ambiguity which would be good to avoid. Namely, we are talking about deletion from the triggers dict, and not deletion from the database.
@kaxil @potiuk i think these objects should be renamed but just not sure if this is considered "breaking" since maybe you could say the triggerer process is "internal"