[GitHub] [airflow] uranusjr commented on a diff in pull request #22958: Support clearing and updating state of individual mapped task instances

[GitBox <gi...@apache.org>]

uranusjr commented on code in PR #22958:
URL: https://github.com/apache/airflow/pull/22958#discussion_r850110542


##########
airflow/api/common/mark_tasks.py:
##########
@@ -118,7 +119,9 @@ def set_state(
     if execution_date and not timezone.is_localized(execution_date):
         raise ValueError(f"Received non-localized date {execution_date}")
 
-    task_dags = {task.dag for task in tasks}
+    t_dags = {task.dag for task in tasks if not isinstance(task, tuple)}
+    t_dags_2 = {item[0].dag for item in tasks if isinstance(item, tuple)}
+    task_dags = t_dags | t_dags_2

Review Comment:
   ```suggestion
       task_dags = {
           task[0].dag if isinstance(task, tuple) else task.dag
           for task in tasks
       }
   ```



##########
airflow/models/dag.py:
##########
@@ -1658,9 +1679,13 @@ def set_task_instance_state(
 
         task = self.get_task(task_id)
         task.dag = self
+        if not map_indexes:
+            map_indexes = [-1]

Review Comment:
   Intuitively I think not passing `map_idnexes` should default to setting states of all task instances of this task, not just -1 (matters if the task is mapped).
   
   Another thing to consider is that, since this function is named `set_task_instance_state` (singular), it probably should only be able to set state to one task instance, i.e. it should only take one map index, not multiple. If we want to allow setting multiple mapped task instances, we should name the function `set_task_state` and deprecate this name.



##########
airflow/models/dag.py:
##########
@@ -1368,6 +1370,7 @@ def _get_task_instances(
         self,
         *,
         task_ids,
+        task_ids_and_map_indexes,

Review Comment:
   Maybe merge these like `set_states`? (same for other arguments below, including the `exclude` ones)
   
   Also maybe add type annotations to these, it’s not easy to tell what should be passed into these at first sight.



##########
airflow/www/views.py:
##########
@@ -2025,6 +2028,14 @@ def clear(self):
         origin = get_safe_url(request.form.get('origin'))
         dag = current_app.dag_bag.get_dag(dag_id)
 
+        map_indexes = request.form.get('map_indexes')

Review Comment:
   There’s `getlist` available for this. To use it we need to format the argument list in the front end in a specific way, but that’s better than manually parsing the list, I think.



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org