pierrejeambrun commented on code in PR #58102:
URL: https://github.com/apache/airflow/pull/58102#discussion_r2577140653
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/common.py:
##########
@@ -44,9 +45,10 @@ def __init__(self, session: Session, request: BulkBody[T]):
self.session = session
self.request = request
- def handle_request(self) -> BulkResponse:
+ def handle_request(self) -> TaskInstanceCollectionResponse:
Review Comment:
Return type is wrong. This class is generic and not tied to a
TaskInstanceResponse.
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/task_instances.py:
##########
@@ -205,8 +219,9 @@ def handle_bulk_create(
def handle_bulk_update(
self, action: BulkUpdateAction[BulkTaskInstanceBody], results:
BulkActionResponse
- ) -> None:
+ ) -> TaskInstanceCollectionResponse:
Review Comment:
You shouldn't update this signature, because it is not consistent with other
bulk operation. (DELETE/CREATE, etc.).
The result is returned via the muatation of the given `results` param. You
can store your results there too. And we know that for a `dry_run` those
results are not actually submitted.
##########
airflow-core/src/airflow/ui/src/components/MarkAs/TaskGroupInstance/MarkTaskGroupInstanceAsDialog.tsx:
##########
@@ -0,0 +1,199 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Flex, Heading, VStack } from "@chakra-ui/react";
+import { useState } from "react";
+import { useTranslation } from "react-i18next";
+import { useParams } from "react-router-dom";
+
+import { useTaskInstanceServiceGetTaskInstances } from "openapi/queries";
+import type {
+ LightGridTaskInstanceSummary,
+ TaskInstanceState,
+ BulkBody_BulkTaskInstanceBody_,
+} from "openapi/requests/types.gen";
+import { ActionAccordion } from "src/components/ActionAccordion";
+import { StateBadge } from "src/components/StateBadge";
+import { Button, Dialog } from "src/components/ui";
+import SegmentedControl from "src/components/ui/SegmentedControl";
+import { useBulkTaskInstances } from "src/queries/useBulkTaskInstances";
+import { useBulkTaskInstancesDryRun } from
"src/queries/useBulkTaskInstancesDryRun";
+
+type Props = {
+ readonly groupTaskInstance: LightGridTaskInstanceSummary;
+ readonly onClose: () => void;
+ readonly open: boolean;
+ readonly state: TaskInstanceState;
+};
+
+const MarkTaskGroupInstanceAsDialog = ({ groupTaskInstance, onClose, open,
state }: Props) => {
+ const { t: translate } = useTranslation();
+ const { dagId = "", runId = "" } = useParams();
+ const groupId = groupTaskInstance.task_id;
+
+ const [selectedOptions, setSelectedOptions] = useState<Array<string>>([]);
+
+ const past = selectedOptions.includes("past");
+ const future = selectedOptions.includes("future");
+ const upstream = selectedOptions.includes("upstream");
+ const downstream = selectedOptions.includes("downstream");
+
+ const [note, setNote] = useState<string>("");
+
+ // Get all task instances in the group
+ const { data: groupTaskInstances } = useTaskInstanceServiceGetTaskInstances(
+ {
+ dagId,
+ dagRunId: runId,
+ taskDisplayNamePattern: groupId,
Review Comment:
We can do this at first but this is not accurate.
A group is not fully identified by querying `taskDisplaNamePattern` with a
`groupId` as a value.
Some groups can specify `prefix=False` and therefore tasks will not match.
We will need to update after https://github.com/apache/airflow/pull/58092 is
merged.
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/common.py:
##########
@@ -55,19 +57,21 @@ def handle_request(self) -> BulkResponse:
if action.action == BulkAction.CREATE:
self.handle_bulk_create(action, results[action.action.value])
elif action.action == BulkAction.UPDATE:
- self.handle_bulk_update(action, results[action.action.value])
+ response = self.handle_bulk_update(action,
results[action.action.value])
elif action.action == BulkAction.DELETE:
self.handle_bulk_delete(action, results[action.action.value])
- return BulkResponse(**results)
+ return response
@abstractmethod
def handle_bulk_create(self, action: BulkCreateAction[T], results:
BulkActionResponse) -> None:
"""Bulk create entities."""
raise NotImplementedError
@abstractmethod
- def handle_bulk_update(self, action: BulkUpdateAction[T], results:
BulkActionResponse) -> None:
+ def handle_bulk_update(
+ self, action: BulkUpdateAction[T], results: BulkActionResponse
+ ) -> TaskInstanceCollectionResponse:
Review Comment:
Same here
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/task_instances.py:
##########
@@ -55,21 +60,23 @@ def _patch_ti_validate_request(
session: SessionDep,
map_index: int | None = -1,
update_mask: list[str] | None = Query(None),
-) -> tuple[SerializedDAG, list[TI], dict]:
+) -> tuple[SerializedDAG, list[TaskInstance], dict]:
dag = get_latest_version_of_dag(dag_bag, dag_id, session)
if not dag.has_task(task_id):
raise HTTPException(status.HTTP_404_NOT_FOUND, f"Task '{task_id}' not
found in DAG '{dag_id}'")
query = (
- select(TI)
- .where(TI.dag_id == dag_id, TI.run_id == dag_run_id, TI.task_id ==
task_id)
- .join(TI.dag_run)
- .options(joinedload(TI.rendered_task_instance_fields))
+ select(TaskInstance)
+ .where(
+ TaskInstance.dag_id == dag_id, TaskInstance.run_id == dag_run_id,
TaskInstance.task_id == task_id
+ )
+ .join(TaskInstance.dag_run)
+ .options(joinedload(TaskInstance.rendered_task_instance_fields))
)
Review Comment:
Nit:
Why changing all TI to `TaskInstance`. It just makes things more verbose,
and that not necessary for the feature you are trying to implement. (also
making the change set bigger)
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/task_instances.py:
##########
@@ -239,28 +254,46 @@ def handle_bulk_update(
map_index=task_instance_body.map_index,
update_mask=None,
)
-
for key, _ in data.items():
if key == "new_state":
- _patch_task_instance_state(
+ updated_tis = _patch_task_instance_state(
task_id=task_instance_body.task_id,
dag_run_id=self.dag_run_id,
dag=dag,
task_instance_body=task_instance_body,
session=self.session,
data=data,
+ commit=self.commit,
)
+ if updated_tis:
+ all_updated_tis.extend(updated_tis)
+
elif key == "note":
_patch_task_instance_note(
task_instance_body=task_instance_body, tis=tis,
user=self.user
)
+ all_updated_tis.extend(tis)
Review Comment:
That's wrong. Structure is not symetrical between patch `new_state` and path
`note`.
The part of `updated_tis` is not correct in this code path.
```suggestion
uptated_tis = all_updated_tis.extend(tis)
```
##########
airflow-core/src/airflow/ui/src/components/MarkAs/TaskGroupInstance/MarkTaskGroupInstanceAsDialog.tsx:
##########
@@ -0,0 +1,199 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Flex, Heading, VStack } from "@chakra-ui/react";
+import { useState } from "react";
+import { useTranslation } from "react-i18next";
+import { useParams } from "react-router-dom";
+
+import { useTaskInstanceServiceGetTaskInstances } from "openapi/queries";
+import type {
+ LightGridTaskInstanceSummary,
+ TaskInstanceState,
+ BulkBody_BulkTaskInstanceBody_,
+} from "openapi/requests/types.gen";
+import { ActionAccordion } from "src/components/ActionAccordion";
+import { StateBadge } from "src/components/StateBadge";
+import { Button, Dialog } from "src/components/ui";
+import SegmentedControl from "src/components/ui/SegmentedControl";
+import { useBulkTaskInstances } from "src/queries/useBulkTaskInstances";
+import { useBulkTaskInstancesDryRun } from
"src/queries/useBulkTaskInstancesDryRun";
+
+type Props = {
+ readonly groupTaskInstance: LightGridTaskInstanceSummary;
+ readonly onClose: () => void;
+ readonly open: boolean;
+ readonly state: TaskInstanceState;
+};
+
+const MarkTaskGroupInstanceAsDialog = ({ groupTaskInstance, onClose, open,
state }: Props) => {
+ const { t: translate } = useTranslation();
+ const { dagId = "", runId = "" } = useParams();
+ const groupId = groupTaskInstance.task_id;
+
+ const [selectedOptions, setSelectedOptions] = useState<Array<string>>([]);
+
+ const past = selectedOptions.includes("past");
+ const future = selectedOptions.includes("future");
+ const upstream = selectedOptions.includes("upstream");
+ const downstream = selectedOptions.includes("downstream");
+
+ const [note, setNote] = useState<string>("");
+
+ // Get all task instances in the group
+ const { data: groupTaskInstances } = useTaskInstanceServiceGetTaskInstances(
+ {
+ dagId,
+ dagRunId: runId,
+ taskDisplayNamePattern: groupId,
Review Comment:
This makes me realize that there is a bug here and in the
`ClearGroupTaskInstanceDialog` probably. Because the results are paginated. So
if the group holds more than the default page size (50) task instances, some
TIs won't be returned by this call and we will fail to clear/update them.
That can be a separate PR because the exisiting code in
`ClearGroupTaskInsatnceDialog` has the same issue.
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/task_instances.py:
##########
@@ -239,28 +254,46 @@ def handle_bulk_update(
map_index=task_instance_body.map_index,
update_mask=None,
)
-
for key, _ in data.items():
if key == "new_state":
- _patch_task_instance_state(
+ updated_tis = _patch_task_instance_state(
task_id=task_instance_body.task_id,
dag_run_id=self.dag_run_id,
dag=dag,
task_instance_body=task_instance_body,
session=self.session,
data=data,
+ commit=self.commit,
)
+ if updated_tis:
+ all_updated_tis.extend(updated_tis)
+
elif key == "note":
_patch_task_instance_note(
task_instance_body=task_instance_body, tis=tis,
user=self.user
)
+ all_updated_tis.extend(tis)
Review Comment:
Also missing the `if` statement present above.
--
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]
