potiuk commented on PR #70295:
URL: https://github.com/apache/airflow/pull/70295#issuecomment-5149553774
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.
`airflow_local_settings.py` splits into two camps. The object-storage
backends merge the IO half of `remote_task_handler_kwargs`:
```python
REMOTE_TASK_LOG = WasbRemoteLogIO(
**cast("dict[str, Any]", { ... } | _io_kwargs)
)
```
The two search backends do not. `OpensearchRemoteLogIO` is constructed with
explicit keyword arguments and no `| _io_kwargs` at all:
```python
REMOTE_TASK_LOG = OpensearchRemoteLogIO(
host=OPENSEARCH_HOST,
port=OPENSEARCH_PORT,
...
log_id_template=OPENSEARCH_LOG_ID_TEMPLATE,
)
```
identical in shape to the `ElasticsearchRemoteLogIO` branch directly above
it. This PR adds the merge anyway, so `from_config` forwards every
non-`FileTaskHandler` key to `OpensearchRemoteLogIO.__init__`.
That is a behaviour change rather than a port. `remote_task_handler_kwargs`
is one config knob whose keys are split between the file handler and the IO
object, so an opensearch deployment can currently put arbitrary keys there for
the file-handler side and nothing reaches the IO class. After this change those
keys are passed to the constructor, and anything it does not accept raises
`TypeError: unexpected keyword argument` at startup — on a configuration that
works today.
Worth noting #70525 hit the same fork and went the other way, with the
reasoning recorded in its docstring:
> Unlike the object-storage backends, this does not merge ``[logging]
remote_task_handler_kwargs`` IO-kwargs, matching the legacy behavior for
Elasticsearch.
Two ways forward, and I don't mind which:
- Drop the merge here so the port is faithful and opensearch stays
consistent with elasticsearch.
- Keep it, if you think the legacy omission was itself an oversight — but
then say so in the PR description and docstring, since it changes startup
behaviour for existing deployments and shouldn't ride in as an incidental
detail of a port.
Everything else here looks like a clean port of the legacy branch.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
