Enable provider-based GCS remote logging resolution - #70504
Merged
shahar1 merged 1 commit intoJul 27, 2026
Merged
Conversation
aaron-y-chen
marked this pull request as ready for review
July 27, 2026 09:56
shahar1
approved these changes
Jul 27, 2026
shahar1
left a comment
Contributor
There was a problem hiding this comment.
LGTM — a faithful port of the gs scheme onto the #67056 dispatch, matching the merged s3 (#69817) and cloudwatch (#69816) migrations line for line, with real-backend e2e evidence in the description. CI is green.
What I checked beyond the diff itself:
- Config parity with the legacy branch.
from_config()reproducesairflow_local_settings.pyL207–L224 exactly — same four keys,expanduseronbase_log_folder,google_key_path→gcp_key_path, and the same| io_kwargsmerge order. Existinggs://configs resolve to an identicalGCSRemoteLogIO. google_key_pathunset (ADC) doesn't tripget_mandatory_value.[logging] google_key_pathdefaults to""rather thanNoneinconfig.yml, so the mandatory getter returns a falsy string instead of raising — same as the legacy path, and consistent with the ADC-only run in your description.- Dropping the derived default conn id is fine. The legacy branch called
_default_conn_name_from(..., "GCSHook"); provider dispatch deliberately doesn't (factory.py: "Provider dispatch does not produce a default conn id … providers that want a backend-specific default can read from their own hook insidefrom_config").GCSRemoteLogIO.hookalready reads[logging] remote_log_conn_iditself and falls back to ADC, and the supervisor's_remote_logging_connprefetch only fires when that key is explicitly set — so no behaviour change, and it matches what #69817 shipped. - Backward compatibility of the new
provider.yamlkey.provider_info.schema.jsondoes not setadditionalProperties: falsein 2.11.1 (or onmain), so theremote-loggingblock is ignored rather than rejected on older Airflow. The Compat 2.11.1 / 3.0.6 / 3.1.8 / 3.2.2 / 3.3.0 jobs confirm it. - The test guards earn their keep. The
hasattr(manager, "remote_logging_handler_by_scheme")skip andpytest.importorskip("airflow._shared.logging.factory")read as defensive at first glance, but they're exactly what lets this class run under the provider-compat jobs against Airflow versions predating #67056.
Smaller observations
providers/google/src/airflow/providers/google/cloud/log/gcs_task_handler.py— this makes three verbatim copies of the ~18-lineremote_task_handler_kwargsparse-and-split block (s3, cloudwatch, now gcs), with several backends still to migrate under #70265. Factoring it out is awkward while providers still support Airflow 2.11 —airflow._shared.logging.factoryisn't importable there — so I'd leave this PR as-is and raise the dedup question on the umbrella issue instead. Purely a note, not a change request.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Contributor
Author
|
Thanks shahar! 😁 |
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes: #70266
related: #70265
related: #67056
Summary
Migrate the
gsremote logging scheme to the provider dispatch introduced by #67056:GCSRemoteLogIO.from_config(), preserving the legacy GCS config mapping and IO kwargs;gsin the Google provider metadata generated fromprovider.yaml.Core and the Task SDK can now resolve GCS logging through
ProvidersManager. The existing legacy path remains as a compatibility fallback.Validation
Tested against a real GCS bucket from Breeze using forwarded user ADC and:
No
remote_log_conn_idorgoogle_key_pathwas set, so the handler used ADC.gsresolved toairflow.providers.google.cloud.log.gcs_task_handler.GCSRemoteLogIO;AIRFLOW_70266_GCS_REMOTE_LOG_E2E_OK, andgcloud storage catreturned the same marker fromattempt=1.log;gs://.../attempt=1.logsource and displayed the marker.Airflow UI reading the task log from GCS after local cleanup:
The uploaded
attempt=1.login the real GCS bucket:Was generative AI tooling used to co-author this PR?
Generated-by: [GPT 5.6 Sol] following the guidelines