Skip to content

Pass X-Trino-Client-Info in trino hook#22535

Merged
eladkal merged 1 commit into
apache:mainfrom
pingzh:pingzh-pass-X-Trino-Client-Info
Mar 27, 2022
Merged

Pass X-Trino-Client-Info in trino hook#22535
eladkal merged 1 commit into
apache:mainfrom
pingzh:pingzh-pass-X-Trino-Client-Info

Conversation

@pingzh

@pingzh pingzh commented Mar 25, 2022

Copy link
Copy Markdown
Contributor

so that in the trino side, it can track back to an airflow task
similar to #22416


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@pingzh

pingzh commented Mar 25, 2022

Copy link
Copy Markdown
Contributor Author

@eladkal please take a look?

@github-actions github-actions Bot added the okay to merge It's ok to merge this PR as it does not require more tests label Mar 25, 2022
@github-actions

Copy link
Copy Markdown
Contributor

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

ca_bundle=extra.get('kerberos__ca_bundle'),
)

http_headers = {"X-Trino-Client-Info": generate_trino_client_info()}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The X-Trino header prefix has been configurable since Trino was rebranded from PrestoSQL:

https://trino.io/docs/current/admin/properties-general.html?highlight=headers#protocol-v1-alternate-header-name

Do you think that its worth supporting the same kind of override here? If a user has overriden the default header prefix then this hardcoded value of X-Trino-Client-Info won't get properly picked up or displayed in the Trino UI.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with not supporting it.
Users of older versions can overide the function and have whatever custom behavior they need to support older versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants