aoelvp94 commented on PR #75566: URL: https://github.com/apache/airflow/pull/75566#issuecomment-6417465449
> Thanks for this. It's a clean port of where #73015 ended up after review: `prefix` and `archive_object` are mutually exclusive, there's no fallback, the console link points at the archive, and `filter="data"` makes the unpack safe. A few points: > > 1. **Sequencing with #73015.** The S3 version is still under review by `o-nikolas`. The two PRs should land with the same semantics: same option shape, same marker file, same error types. Can we settle the S3 design first and then align this one? That way we don't end up with two subtly different archive modes. > 2. **Exception type** (`gcs.py:108`). A missing bucket or prefix raises `ValueError`, but a missing archive raises `AirflowNotFoundException`. We're moving away from the `AirflowException` hierarchy in providers. `ValueError`, or `FileNotFoundError`, would match the neighbouring checks. > 3. **The comment overstates the swap** (`gcs.py:203`). "A partially staged bundle is never observable" isn't quite right. Between the two renames, `gcs_dags_dir` doesn't exist, which is the same point raised on the S3 docs. Readers don't take `self.lock()`, so a parse that lists the directory during that gap will see nothing. Please reword the comment to say what's actually guaranteed: no partially unpacked tree, but a brief gap where the directory is missing. > 4. **The raw client bypasses `GCSHook`** (`gcs.py:107`, `175`, `191`). Going through `get_conn().bucket().blob()` loses the retry loop in `GCSHook.download(num_max_attempts=...)`. It also costs two metadata calls per refresh after init (`exists()` plus `reload()`). Two suggestions: > > * Drop the `exists()` probe and translate `NotFound` from `reload()`. > * Pin the download to the generation you just read (`if_generation_match=blob.generation`). Today an overwrite between `reload()` and the download stamps the new contents with the old ETag. That heals itself on the next refresh, but it costs an extra full download. > 5. **Unreachable guard** (`gcs.py:173`). The `if not archive_object: raise ValueError` exists only to narrow the type. Passing `archive_object` into `_refresh_from_archive()` from `refresh()` would remove the dead branch. > 6. **Tests.** > > * The hook mocks are bare `MagicMock()`. The repo convention is `spec`/`autospec`, e.g. `MagicMock(spec=GCSHook)`. > * The download-counting monkeypatch in `test_refresh_from_archive_skips_unchanged_etag` is fairly convoluted. A `mock.patch.object(_FakeBlob, "download_to_filename", autospec=True, side_effect=...)` spy would read more simply. > * The empty-ETag path (no marker written, so it always restages) has no test. > 7. Nit: the docs say "raises at startup". It's actually raised when the bundle is constructed, so it fails on bundle config load. That's fine, just slightly imprecise. > > None of these block the approach. Points 1–3 are what I'd like resolved before merge. > > Drafted-by: Claude Opus 5.5; reviewed by `potiuk` before posting Thanks for the review. The GCS-specific notes are in separate commits on this PR. 1. Agreed the two PRs should land with the same semantics. Jason Liu asked on #73015 whether this should be a separate `S3ArchiveDagBundle`, partly so an archive object can carry a version id. I'd like to settle that there before mirroring anything further. My preference is to keep the optional argument on the existing class: with it unset, behavior is unchanged, and a second class repeats the hook, the lock, and the local path. Versioning fits an archive object, and `supports_versioning` is still `False`. I'd rather add that as a follow-up than block staging on it. Happy to split the class if that follow-up is what we want. 2. A missing archive now raises `ValueError`, same as a missing bucket or prefix. https://github.com/apache/airflow/pull/75566/commits/314d316480303ca96b24fb1f116028b01358c734 3. The swap comment no longer says the directory is always observable. It says a half-unpacked tree is never left behind, and that there is a short gap between the two renames where the directory does not exist. Readers do not take the bundle lock. https://github.com/apache/airflow/pull/75566/commits/f8e46b55c4b8cffcc2c5edf4868618a63c79ead4 4. The `exists()` probe is gone. `NotFound` from `reload()` is raised as `ValueError`, so a refresh does one metadata read. The download passes `if_generation_match` for the generation just read, and a mismatch leaves the previous tree in place. I kept the blob download rather than `GCSHook.download`, because that method cannot pass `if_generation_match`. Its default is also a single attempt, so the retry loop only applies when a caller asks for more. I can teach `GCSHook.download` about the generation match if we want both. https://github.com/apache/airflow/pull/75566/commits/1b0cf948b8059217a79812665191be0be189f022 5. `refresh()` passes `archive_object` into the staging method. The guard that only narrowed the type is gone. https://github.com/apache/airflow/pull/75566/commits/a3b4c3ca9d26ba18664cec3356e483383b265828 6. The archive tests use `MagicMock(spec=GCSHook)`. The unchanged-ETag test spies on `download_to_filename`. An empty ETag writes no marker and stages again on the next refresh. https://github.com/apache/airflow/pull/75566/commits/840fbdef9886eb4a0fa95147e21f561ad23328c6 7. The docs now say the conflict is raised when the bundle is constructed, which is when the bundle config is loaded. https://github.com/apache/airflow/pull/75566/commits/26dc8477c220ddd663fc01f022d06f8662ab52d4 I have not changed #73015 yet. The same edits belong there once the class shape is settled: `ValueError` for a missing archive, the same swap wording, drop `check_for_key` and translate a 404 from `head_object`, and `If-Match` on the download. The S3 code comment still says a partial tree is never observable. -- 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]
