kaxil commented on code in PR #75428:
URL: https://github.com/apache/airflow/pull/75428#discussion_r4354591173
##########
airflow-core/docs/img/diagram_task_lifecycle.py:
##########
@@ -178,15 +178,15 @@ def generate_task_lifecycle_diagram():
cond_scheduled_skip >> Edge(label="NO") >> cond_task_def_existence
(cond_scheduled_skip >> Edge(label="YES") >> state_skipped,)
(cond_task_def_existence >> Edge(label="NO") >> state_removed,)
- (cond_task_def_existence >> Edge(label="YES") >> state_scheduled,)
+ (cond_task_def_existence >> Edge(label="YES") >> cond_trigger_task_1,)
state_removed >> cond_task_restore
cond_task_restore >> Edge(label="NO") >> state_removed
cond_task_restore >> Edge(label="YES") >> state_none
- state_scheduled >> component_executor >> state_queued >>
cond_trigger_task_1
- cond_trigger_task_1 >> Edge(label="NO") >> component_worker
+ cond_trigger_task_1 >> Edge(label="NO") >> state_scheduled
cond_trigger_task_1 >> Edge(label="YES") >> component_triggerer
+ state_scheduled >> component_executor >> state_queued >>
component_worker
component_triggerer >> state_deferred >> cond_trigger_task_2
- cond_trigger_task_2 >> Edge(label="NO") >> component_scheduler
+ cond_trigger_task_2 >> Edge(label="NO") >> state_scheduled
Review Comment:
The `awaiting_input` edge a few lines down (`>> component_scheduler`) has
the same problem. Both the HITL response route and the scheduler's timeout
sweep resume through `handle_event_submit`, so that task also lands in
`scheduled` directly. Could you point that arrow at `state_scheduled` too, so
the two resume paths read the same?
##########
airflow-core/docs/img/diagram_task_lifecycle.py:
##########
@@ -178,15 +178,15 @@ def generate_task_lifecycle_diagram():
cond_scheduled_skip >> Edge(label="NO") >> cond_task_def_existence
(cond_scheduled_skip >> Edge(label="YES") >> state_skipped,)
(cond_task_def_existence >> Edge(label="NO") >> state_removed,)
- (cond_task_def_existence >> Edge(label="YES") >> state_scheduled,)
+ (cond_task_def_existence >> Edge(label="YES") >> cond_trigger_task_1,)
state_removed >> cond_task_restore
cond_task_restore >> Edge(label="NO") >> state_removed
cond_task_restore >> Edge(label="YES") >> state_none
- state_scheduled >> component_executor >> state_queued >>
cond_trigger_task_1
- cond_trigger_task_1 >> Edge(label="NO") >> component_worker
+ cond_trigger_task_1 >> Edge(label="NO") >> state_scheduled
cond_trigger_task_1 >> Edge(label="YES") >> component_triggerer
+ state_scheduled >> component_executor >> state_queued >>
component_worker
component_triggerer >> state_deferred >> cond_trigger_task_2
- cond_trigger_task_2 >> Edge(label="NO") >> component_scheduler
+ cond_trigger_task_2 >> Edge(label="NO") >> state_scheduled
cond_trigger_task_2 >> Edge(label="YES") >> cond_task_complete_1
Review Comment:
Both diamonds are labelled "A triggerer task?", but with this change they
ask different things. The first one is `start_from_trigger`, and this one only
makes sense as "does the trigger end the task?" (`end_from_trigger`, a
`TaskSuccessEvent` or `TaskFailureEvent`). Someone tracing a
`start_from_trigger` sensor will answer YES here and never see it go back to a
worker for `next_method`. Worth relabelling this one while you're in here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
