potiuk commented on code in PR #26658:
URL: https://github.com/apache/airflow/pull/26658#discussion_r1015864927
##########
airflow/www/views.py:
##########
@@ -2047,14 +2053,42 @@ def clear(self):
recursive = request.form.get('recursive') == "true"
only_failed = request.form.get('only_failed') == "true"
- task_ids: list[str | tuple[str, int]]
- if map_indexes is None:
- task_ids = [task_id]
- else:
- task_ids = [(task_id, map_index) for map_index in map_indexes]
+ task_ids: list[str | tuple[str, int]] = []
+
+ end_date = execution_date if not future else None
+ start_date = execution_date if not past else None
+
+ if group_id is not None:
+ task_group_dict = dag.task_group.get_task_group_dict()
+ task_group = task_group_dict.get(group_id)
+ if task_group is None:
+ return redirect_or_json(
+ origin, msg=f"TaskGroup {group_id} could not be found",
status="error", status_code=404
+ )
+ task_ids = task_ids_or_regex = [t.task_id for t in
task_group.iter_tasks()]
+
+ # Lock the related dag runs to prevent from possible dead lock.
+ # https://github.com/apache/airflow/pull/26658
+ dag_runs_query = session.query(DagRun).filter(DagRun.dag_id ==
dag_id).with_for_update()
+ if start_date is None and end_date is None:
+ dag_runs_query = dag_runs_query.filter(DagRun.execution_date
== start_date)
+ else:
+ if start_date is not None:
+ dag_runs_query =
dag_runs_query.filter(DagRun.execution_date >= start_date)
+
+ if end_date is not None:
+ dag_runs_query =
dag_runs_query.filter(DagRun.execution_date <= end_date)
+
+ _ = dag_runs_query.all()
Review Comment:
Yeah I have a feeling that locking especially mutliple DagRuns will cause
long running queries (more than few seconds) with multiple DagRuns locked and
secondly it might cause UI problems when such clearing willl be a long running
operation for many future/past dagruns and multiple tasks belonging to a task
group.
I think we should get a good feeling on how long such DagRun lock query can
be - when we have big installation will it keep the locks for milliseconds?
single seconds? 10-20 seconds? I am afraid (but this is just a guess) it might
take longer. Also when you lock such multiple DagRuns and scheduler already
keeps any of the DagRuns because it is scheduling that particular DagRun - this
query will run for as long as scheduler (or maybe - depending on lock
priorities - multiple schedulers) will keep them. I can easily imagine this
query might "hang" for multiple seconds while waiting for single DagRun lock
release. This is not a problem for schedulers because they use "skip locked"
but this is a problem here.
The locks are not applied "atomically" they are likely applied one-by-one.
And schedulers use "lock skip locked" in a tight loop. What happens if such a
row lock is set by one scheduler and then another scheduler schedules some next
one, the first one releases first lock but will get the next one etc. etc.
etc. This might effectively turn that query into multiple minutes query.
Let me just voice again my concerns here (not blocking but something that
should be very well thought here) - when you have such multi-entity operations
which interfere with running schedulers, I think it's inevitable you will run
into competing for DagRuns and it has potential to run for even multiple
minutes. When you run an operation from a UI that can potentially last more
than fews seconds there are two ways the UI problems might appear.
a) You run this operation "synchronously" - you lock everything for the time
of opertion and UI gets "blocked" during this operation. This is difficult to
achive and it is prone to user pressing "back" and re-running the queries and
other operations that might again lock/deadlock.
b) you let it return quickly and keep it running in the background - but
then you need to provide some kind of feedback operation and you need to have
an entity which will keep running the query and provide feedback and UI to
handle those.
Not sure if we are prepared for either of those scenarios.
Maybe my concerns are not founded and in vast majority of cases those
operations will be short, but I think we should also think about big use cases.
--
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]
