Add OSSRemoteLogIO.from_config and register oss remote logging scheme - #70682
Conversation
potiuk
left a comment
There was a problem hiding this comment.
Faithful port, and it gets right a subtlety that would have been easy to "tidy up" incorrectly: legacy passes remote_base_log_folder to OSSRemoteLogIO unstripped, keeping the oss:// prefix, whereas the wasb branch strips wasb://. This preserves that asymmetry, and the test pins it:
assert subject.remote_base == "oss://bucket/remote/log/location"Normalising the two for consistency is the tempting mistake here, so good that it didn't happen. The three fields match legacy and | io_kwargs is correct, since oss is one of the object-storage backends where legacy merges.
One observation I chased across this and the wasb port, recorded here mostly so it isn't rediscovered later. Legacy calls _default_conn_name_from(...) immediately before constructing the IO object:
_default_conn_name_from("airflow.providers.alibaba.cloud.hooks.oss", "OSSHook")Not carrying that into from_config is correct — it sets the module-global DEFAULT_REMOTE_CONN_ID in airflow_local_settings.py, which is part of the legacy settings-module contract, and the IO class takes no connection id, so there is nothing here to set.
It does leave a question for the dispatch mechanism rather than for this PR: under legacy that global was seeded from each hook's default_conn_name, so deployments moving to remote_base_log_folder = oss://... may not get that default any more. Worth confirming core handles it somewhere, but not something to hold this up for.
Rebased onto main before merging.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Signed-off-by: baha-bouali <baha.bouali2020@gmail.com>
7d7f648 to
d7a10a0
Compare
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Part of #70265 (related: #67056, migrates #70269).
Why
#67056 decoupled remote logging from the hardcoded
if/elifchain inairflow_local_settings.py: core and the Task SDK now resolve the remote loghandler via
ProvidersManagerdispatch, keyed on the URL scheme of[logging] remote_base_log_folder, and instantiate the provider's classthrough a no-arg
from_config()classmethod. Iffrom_config()is missing orraises, the shared factory falls back to the legacy branch — so this
migration is non-breaking by construction.
This PR migrates the
ossscheme, following the same pattern already mergedfor S3 (#69817) and CloudWatch (#69816).
Was generative AI tooling used to co-author this PR?
Assisted by Claude Sonnet 5, every line reviewed by the submitter.
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.