kaxil commented on code in PR #67833:
URL: https://github.com/apache/airflow/pull/67833#discussion_r3333837270
##########
task-sdk/src/airflow/sdk/execution_time/comms.py:
##########
@@ -566,24 +566,24 @@ def from_variable_response(cls, variable_response:
VariableResponse) -> Variable
return cls(**variable_response.model_dump(exclude_defaults=True),
type="VariableResult")
-class TaskStateResult(TaskStateResponse):
- """Response to GetTaskState; wraps the generated API response for
supervisor to worker comms."""
+class TaskStoreResult(TaskStoreResponse):
Review Comment:
This rename breaks two message-type sync tests that aren't in this PR's diff:
- `airflow-core/tests/unit/dag_processing/test_processor.py` (~line 1990)
- `airflow-core/tests/unit/jobs/test_triggerer_job.py` (~line 1985)
Both build the live type set from `get_args(ToTask)` /
`get_args(ToSupervisor)` and diff it against hardcoded allowlists
(`in_supervisor_but_not_in_manager`,
`in_task_runner_but_not_in_dag_processing_process`) that still carry the old
names: `GetTaskState`, `SetTaskState`, `DeleteTaskState`, `ClearTaskState`, the
eight `*AssetState{ByName,ByUri}` request types, plus `TaskStateResult` and
`AssetStateResult`. After the rename those names appear zero times in the live
unions, so the new `*Store` types fall into `task_diff` / `supervisor_diff` and
the `assert not task_diff` / `assert not supervisor_diff` checks fire.
Fix: rename those allowlist entries to their `*Store` counterparts in both
files. Keep `TaskState`, `GetTaskStates`, and `TaskStatesResult` -- those are
task-lifecycle types and aren't renamed by this PR.
##########
airflow-core/src/airflow/state/metastore.py:
##########
@@ -83,25 +83,25 @@ def _build_upsert_stmt(
return stmt
-class MetastoreStateBackend(BaseStateBackend):
+class MetastoreStateBackend(BaseStoreBackend):
Review Comment:
The base class is renamed to `BaseStoreBackend` and the in-memory test
backend to `InMemoryStoreBackend`, but the concrete default stays
`MetastoreStateBackend`, so a "StateBackend" now subclasses a "StoreBackend".
The `airflow.state` package and `metastore` module names also keep "state".
Intentional (the config default path
`airflow.state.metastore.MetastoreStateBackend` is user-facing, so renaming it
carries a real cost), or a missed spot? A one-line note in the PR would settle
it either way.
--
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]
