opensearch: generalize extra parameters - #65613
Conversation
957780c to
1d3ef41
Compare
1d3ef41 to
2af56cd
Compare
potiuk
left a comment
There was a problem hiding this comment.
I think it is too generic as it might update things we do not want to update via extras. Extras are potentially more sensitive than Hook parameters, because they are configurable by "connection editing" users and not "Dag Authors" - we never pass extras directly to hoo params/args we are always explicit which extras should be passed.
|
okay I will specifically define all the parameters in https://opensearch-project.github.io/opensearch-py/_modules/opensearchpy/connection/base.html#Connection . I see that the |
1ff244d to
3ba6c52
Compare
|
@potiuk comments addressed |
3ba6c52 to
47ee0a7
Compare
47ee0a7 to
db7bfe8
Compare
|
@potiuk could you check whenever you have the time? thanks! |
|
@DonHaul — A reviewer (@potiuk) has requested changes on this PR, so I've removed the Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
|
@DonHaul — you've pushed commits since the changes-requested review, but the review threads are still open. If you've addressed the feedback, reply to confirm and nudge the reviewer for another look; otherwise let us know what's still outstanding. Thanks! See the PR quality criteria. Automated first-pass triage note drafted by an AI-assisted tool — may get things wrong; once addressed, a real Apache Airflow maintainer takes the next look. (why automated) Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting |
potiuk
left a comment
There was a problem hiding this comment.
This resolves my objection cleanly. Instead of forwarding connection extras
generically, you've moved to an explicit, named allowlist — url_prefix,
timeout, headers, http_compress, opaque_id, scheme — each declared in
OpenSearchClientArguments, read by name in __init__, and conditionally passed
to the client only when set. That's exactly the "be explicit about which extras
reach the hook arguments" pattern; there's no arbitrary passthrough of
connection-editor-controlled values, and every forwarded key maps to a known
opensearchpy client argument. test_hook_extra_params pins the mapping, and
CI is green. Thanks for reworking it — LGTM.
This review was drafted by an AI-assisted tool and confirmed by an 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 Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
The current OpenSearch hook, does not include the possibility to set arguments for the opensearch client that may be important for some users such as
http_compressandurl_prefixetc.opensearchpylibrary supports many more arguments, includingkwargs- see hereAs such I've generalized the hook so all the parameters in
extraare passed to the opensearch clientWas 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.