kaxil commented on code in PR #73198:
URL: https://github.com/apache/airflow/pull/73198#discussion_r4472937865
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -670,7 +670,7 @@ def get_sequence_item(
resp =
self.client.get(f"xcoms/{dag_id}/{run_id}/{task_id}/{key}/item/{offset}")
except ServerResponseError as e:
if e.response.status_code == HTTPStatus.NOT_FOUND:
- log.error(
+ log.warning(
Review Comment:
A 404 here isn't really a miss. `LazyXComIterator` keeps asking for the next
offset until it gets an `IndexError`, so every `for v in values` or
`sum(values)` over a mapped upstream ends with one of these, and at warning it
still shows up on every healthy map/reduce run. Can this one and the one in
`get()` above be `log.debug`, like the Connection and Variable not-found
branches in this client?
##########
task-sdk/tests/task_sdk/api/test_client.py:
##########
@@ -1045,6 +1045,54 @@ def handle_request(request: httpx.Request) ->
httpx.Response:
key="key",
)
+ def test_xcom_get_not_found_logs_warning(self, cap_structlog):
+ def handle_request(request: httpx.Request) -> httpx.Response:
+ if request.url.path == "/xcoms/dag_id/run_id/task_id/key":
+ return httpx.Response(status_code=404, json={"detail": "XCom
not found"})
+ return httpx.Response(status_code=400, json={"detail": "Bad
Request"})
+
+ client = make_client(transport=httpx.MockTransport(handle_request))
+
+ with cap_structlog.at_level("warning"):
+ result = client.xcoms.get(
+ dag_id="dag_id",
+ run_id="run_id",
+ task_id="task_id",
+ key="key",
+ )
+
+ assert result == XComResponse(key="key", value=None)
+ assert {"log_level": "warning", "event": "XCom not found"} in
cap_structlog
Review Comment:
The return-value assertions in both tests are the useful part, since nothing
covered the 404 fallback before. The `log_level` check just restates the source
line and would need editing again if the level changes. Could these be named
after the fallback (e.g. `test_xcom_get_404_returns_none_value`) and drop the
level assertion?
##########
task-sdk/tests/task_sdk/api/test_client.py:
##########
@@ -1045,6 +1045,54 @@ def handle_request(request: httpx.Request) ->
httpx.Response:
key="key",
)
+ def test_xcom_get_not_found_logs_warning(self, cap_structlog):
+ def handle_request(request: httpx.Request) -> httpx.Response:
Review Comment:
Main moved the Task SDK to `httpx2` (#73116) after this branch was cut, so
once merged these tests reference an undefined `httpx` (ruff F821, and a
`NameError` when they run). Adding `import httpx` won't work either since the
task-sdk ruff config bans it. Could you rebase and switch them to `httpx2` like
the neighbouring tests?
--
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]
