Uh oh!
There was an error while loading. Please reload this page.
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:
assertsubject.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
d7a10a0CompareUh oh!
There was an error while loading. Please reload this page.
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
…apache#70682) Signed-off-by: baha-bouali <baha.bouali2020@gmail.com>
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.