kaxil commented on code in PR #76381:
URL: https://github.com/apache/airflow/pull/76381#discussion_r4504512323
##########
providers/common/ai/src/airflow/providers/common/ai/durable/storage.py:
##########
@@ -92,8 +92,16 @@ def _load_cache(self) -> dict[str, Any]:
path = self._get_path()
try:
self._cache = json.loads(path.read_text())
- except (FileNotFoundError, OSError, json.JSONDecodeError, ValueError):
+ except FileNotFoundError:
self._cache = {}
+ except OSError:
Review Comment:
1.0.0 and 1.1.0 read the S3 403 for a role without `s3:ListBucket` as an
empty journal, so those setups now fail every attempt after upgrading, starting
with the first read. Can this get a `.. note::` under the Changelog header in
`providers/common/ai/docs/changelog.rst`? Something like: on Airflow < 3.3, a
journal read failure or a corrupt journal now fails the attempt, and S3 roles
need `s3:ListBucket` before upgrading.
##########
providers/common/ai/src/airflow/providers/common/ai/durable/storage.py:
##########
@@ -92,8 +92,16 @@ def _load_cache(self) -> dict[str, Any]:
path = self._get_path()
try:
self._cache = json.loads(path.read_text())
- except (FileNotFoundError, OSError, json.JSONDecodeError, ValueError):
+ except FileNotFoundError:
self._cache = {}
+ except OSError:
+ log.warning(
+ "Durable: could not read the journal; failing this attempt so
finished steps don't run again"
+ )
+ raise
+ except (json.JSONDecodeError, ValueError):
+ log.warning("Durable: journal is corrupt; failing this attempt so
finished steps don't run again")
Review Comment:
The docs now tell users to inspect, restore or delete the journal, but the
file is named by a sha256 of the task instance and neither warning says which
file it is. Can both warnings pass `path=str(path)`?
##########
providers/common/ai/docs/durable_execution.rst:
##########
@@ -118,6 +118,17 @@ worker can read the journal:
[common.ai]
durable_cache_path = s3://my-bucket/airflow/durable-cache
+For S3, the IAM role must also have ``s3:ListBucket`` permission on the
bucket, in
Review Comment:
These two paragraphs land between the `durable_cache_path` example and
"Without the option, the task fails with:", so "the option" no longer has
anything nearby to point at. Could they move below that `ValueError` block?
--
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]
