pierrejeambrun commented on code in PR #44383:
URL: https://github.com/apache/airflow/pull/44383#discussion_r1860717373
##########
tests/api/common/test_airflow_health.py:
##########
@@ -33,8 +33,12 @@
@patch("airflow.api.common.airflow_health.SchedulerJobRunner.most_recent_job",
return_value=None)
@patch("airflow.api.common.airflow_health.TriggererJobRunner.most_recent_job",
return_value=None)
@patch("airflow.api.common.airflow_health.DagProcessorJobRunner.most_recent_job",
return_value=None)
+@patch("airflow.api.common.airflow_health.conf.getboolean", return_value=True)
Review Comment:
We should mock the appropriate conf option `standalone_dag_processor`
instead of the whole `getboolean` method. That could incure undesired side
effect for other conf options.
`@conf_vars(...)`
##########
airflow/ui/src/pages/Dashboard/Health/Health.tsx:
##########
@@ -54,15 +54,14 @@ export const Health = () => {
status={data?.triggerer.status}
title="Triggerer"
/>
- {/* TODO: Update this to match the API when we move the config check
to the API level */}
- {data?.dag_processor.status === undefined ? undefined : (
+ {data && "dag_processor" in data ? (
Review Comment:
I think this could be simplified, cf following suggestions. (remove ternary,
and optional chaining operator)
```suggestion
{data?.dag_processor && (
```
##########
airflow/ui/openapi-gen/requests/schemas.gen.ts:
##########
@@ -2823,11 +2823,18 @@ export const $HealthInfoSchema = {
$ref: "#/components/schemas/TriggererInfoSchema",
},
dag_processor: {
- $ref: "#/components/schemas/DagProcessorInfoSchema",
+ anyOf: [
+ {
+ $ref: "#/components/schemas/DagProcessorInfoSchema",
+ },
+ {
+ type: "null",
+ },
+ ],
Review Comment:
To achieve that, you can take a look at fast api `response_model=Item,
response_model_exclude_unset=True`
This should allow you to actually just return the plain dictionnary in your
endpoint. (remove the returned type annotation as well of the endpoint)
##########
tests/api_fastapi/core_api/routes/public/test_monitor.py:
##########
@@ -103,3 +103,57 @@ def test_unhealthy_metadatabase_status(self,
most_recent_job_mock, test_client):
assert "unhealthy" == body["metadatabase"]["status"]
assert body["scheduler"]["latest_scheduler_heartbeat"] is None
+
+
+class TestGetHealthDagProcessorEnabled(TestMonitorEndpoint):
Review Comment:
We are testing the same endpoint, I think this should be part of the
`TestGetHealth` class.
##########
airflow/api_fastapi/core_api/datamodels/monitor.py:
##########
@@ -49,4 +51,8 @@ class HealthInfoSchema(BaseModel):
metadatabase: BaseInfoSchema
scheduler: SchedulerInfoSchema
triggerer: TriggererInfoSchema
- dag_processor: DagProcessorInfoSchema
+ dag_processor: DagProcessorInfoSchema | None = None
+
+ @model_serializer()
+ def serialize(self):
+ return {k: v for k, v in self if not (k == "dag_processor" and v is
None)}
Review Comment:
If we use `response_model_exclude_unset=True`, we won't need it anymore.
Also just a
[field_serializer](https://docs.pydantic.dev/latest/api/functional_serializers/#pydantic.functional_serializers.field_serializer)
on the `dag_processor` is most likely enough.
##########
airflow/ui/src/pages/Dashboard/Health/Health.tsx:
##########
@@ -54,15 +54,14 @@ export const Health = () => {
status={data?.triggerer.status}
title="Triggerer"
/>
- {/* TODO: Update this to match the API when we move the config check
to the API level */}
- {data?.dag_processor.status === undefined ? undefined : (
+ {data && "dag_processor" in data ? (
<HealthTag
isLoading={isLoading}
- latestHeartbeat={data.dag_processor.latest_dag_processor_heartbeat}
- status={data.dag_processor.status}
+
latestHeartbeat={data.dag_processor?.latest_dag_processor_heartbeat}
+ status={data.dag_processor?.status}
title="Dag Processor"
/>
- )}
+ ) : undefined}
Review Comment:
```suggestion
)
```
##########
airflow/ui/openapi-gen/requests/schemas.gen.ts:
##########
@@ -2823,11 +2823,18 @@ export const $HealthInfoSchema = {
$ref: "#/components/schemas/TriggererInfoSchema",
},
dag_processor: {
- $ref: "#/components/schemas/DagProcessorInfoSchema",
+ anyOf: [
+ {
+ $ref: "#/components/schemas/DagProcessorInfoSchema",
+ },
+ {
+ type: "null",
+ },
+ ],
Review Comment:
This is not accurate. `dag_processor` is not either `DagProcessorInfoSchema`
or `None`.
It's either defined and here with `DagProcessorInfoSchema` or completely
missing from the body.
##########
airflow/ui/src/pages/Dashboard/Health/Health.tsx:
##########
@@ -54,15 +54,14 @@ export const Health = () => {
status={data?.triggerer.status}
title="Triggerer"
/>
- {/* TODO: Update this to match the API when we move the config check
to the API level */}
- {data?.dag_processor.status === undefined ? undefined : (
+ {data && "dag_processor" in data ? (
<HealthTag
isLoading={isLoading}
- latestHeartbeat={data.dag_processor.latest_dag_processor_heartbeat}
- status={data.dag_processor.status}
+
latestHeartbeat={data.dag_processor?.latest_dag_processor_heartbeat}
+ status={data.dag_processor?.status}
Review Comment:
```suggestion
latestHeartbeat={data.dag_processor.latest_dag_processor_heartbeat}
status={data.dag_processor.status}
```
##########
airflow/api_fastapi/core_api/routes/public/monitor.py:
##########
@@ -27,4 +27,4 @@
@monitor_router.get("/health")
def get_health() -> HealthInfoSchema:
airflow_health_status = get_airflow_health()
- return HealthInfoSchema.model_validate(airflow_health_status)
+ return HealthInfoSchema.model_validate(airflow_health_status,
from_attributes=True)
Review Comment:
`from_attributes` should be removed.
Because it is now in the model configuration. This is done automatically.
--
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]
