Add OpensearchRemoteLogIO.from_config and register opensearch scheme - #70295
Add OpensearchRemoteLogIO.from_config and register opensearch scheme#70295FrankYang0529 wants to merge 1 commit into
Conversation
4d1174b to
de1fb45
Compare
|
I reviewed this alongside the four sibling PRs doing the same port for other providers (#70525 elasticsearch, #70301 wasb, #70682 oss, #70549 stackdriver), and this one diverges from the legacy behaviour it is porting.
REMOTE_TASK_LOG = WasbRemoteLogIO(
**cast("dict[str, Any]", { ... } | _io_kwargs)
)The two search backends do not. REMOTE_TASK_LOG = OpensearchRemoteLogIO(
host=OPENSEARCH_HOST,
port=OPENSEARCH_PORT,
...
log_id_template=OPENSEARCH_LOG_ID_TEMPLATE,
)identical in shape to the That is a behaviour change rather than a port. Worth noting #70525 hit the same fork and went the other way, with the reasoning recorded in its docstring:
Two ways forward, and I don't mind which:
Everything else here looks like a clean port of the legacy branch. Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
jason810496
left a comment
There was a problem hiding this comment.
Thanks! I agreed with Jarek's comment overall, we need to follow the existing pattern in
airflow/airflow-core/src/airflow/config_templates/airflow_local_settings.py
Lines 357 to 387 in 182919a
| "base_log_folder", | ||
| } | ||
| io_kwargs = {k: v for k, v in remote_task_handler_kwargs.items() if k not in fth_params} | ||
| port = conf.get("opensearch", "port", fallback="") |
There was a problem hiding this comment.
| port = conf.get("opensearch", "port", fallback="") | |
| port = conf.getint("opensearch", "port", fallback=9200) |
There was a problem hiding this comment.
I prefer to use port = conf.get("opensearch", "port", fallback=""). The default value for port field is empty string. If we use conf.getint("opensearch", "port", fallback=9200), it doesn't fallback to 9200 port.
airflow/providers/opensearch/provider.yaml
Lines 110 to 116 in 4899e90
It shows error like following:
ImportError: Unable to load logging config from airflow.config_templates.airflow_local_settings.DEFAULT_LOGGING_CONFIG due to: AirflowConfigException:Failed to convert value to int. Please check "PORT" key in "opensearch" section. Current value: "".
The original airflow_local_setting.py path has similar problem. I can create another PR to fix it after we get conclusion here.
There was a problem hiding this comment.
How about using port = conf.get("opensearch", "port", fallback="") or 9200 here and the airflow_local_setting.py.
Yes, let's create another PR to fix the same semantic error in airflow_local_setting.py, thanks.
There was a problem hiding this comment.
Thank you. I use 9200 as default in port=int(port) if port else 9200 and keep port = conf.get("opensearch", "port", fallback="").
de1fb45 to
a1053c0
Compare
Thanks @potiuk and @jason810496's review. I prefer to keep behavior consistent with existing pattern. |
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the update. Nice catch! I didn't know this.
| "base_log_folder", | ||
| } | ||
| io_kwargs = {k: v for k, v in remote_task_handler_kwargs.items() if k not in fth_params} | ||
| port = conf.get("opensearch", "port", fallback="") |
There was a problem hiding this comment.
How about using port = conf.get("opensearch", "port", fallback="") or 9200 here and the airflow_local_setting.py.
Yes, let's create another PR to fix the same semantic error in airflow_local_setting.py, thanks.
Signed-off-by: PoAn Yang <payang@apache.org>
a1053c0 to
21df2b0
Compare
Why
#67056 decoupled remote logging from the hardcoded branches in
airflow_local_settings.py: core and the Task SDK now resolve the handler via ProvidersManager dispatch on the[logging] remote_base_log_folderURL scheme. This migrates theopensearchscheme.How
OpensearchRemoteLogIO.from_config()reading the[opensearch]section — a mirror of the legacy branch, so the user-facing config surface is unchanged.opensearchscheme inprovider.yaml.Verify
uv run --project providers/opensearch pytest providers/opensearch/tests/unit/opensearch/log/test_os_task_handler.pyWas generative AI tooling used to co-author this PR?
{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.