Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass X-Presto-Client-Info in presto hook #22416

Merged
merged 1 commit into from
Mar 24, 2022

Conversation

pingzh
Copy link
Contributor

@pingzh pingzh commented Mar 22, 2022

so that in the presto side, it can track back to an airflow task, according to this doc https://prestodb.io/docs/current/develop/client-protocol.html


^ 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 force-pushed the pingzh-pass-X-Presto-Client-Info branch 2 times, most recently from bd2552a to 4ce86ea Compare March 22, 2022 17:08
@pingzh pingzh closed this Mar 22, 2022
@pingzh pingzh reopened this Mar 22, 2022
@pingzh pingzh requested a review from potiuk March 22, 2022 17:43
@pingzh
Copy link
Contributor Author

pingzh commented Mar 22, 2022

@potiuk any suggestions that I can fix this https://github.com/apache/airflow/runs/5648830794?check_suite_focus=true#step:11:1039

since there is not DEFAULT_FORMAT_PREFIX in 2.1.1 https://github.com/apache/airflow/blob/2.1.1/airflow/utils/operator_helpers.py#L20

There were errors when importing Providers!
This test is run to check Provider's compatibility with Airflow 2.1.0
So this error might mean that your providers are not Airflow 2.1.0 compatible

Detailed errors:

----------------------------------------
Exception when importing: airflow.providers.google.cloud.example_dags.example_presto_to_gcs


Traceback (most recent call last):
  File "/opt/airflow/dev/import_all_classes.py", line 90, in import_all_classes
    _module = importlib.import_module(modinfo.name)
  File "/usr/local/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name, package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/usr/local/lib/python3.7/site-packages/airflow/providers/google/cloud/example_dags/example_presto_to_gcs.py", line 32, in <module>
    from airflow.providers.google.cloud.transfers.presto_to_gcs import PrestoToGCSOperator
  File "/usr/local/lib/python3.7/site-packages/airflow/providers/google/cloud/transfers/presto_to_gcs.py", line 24, in <module>
    from airflow.providers.presto.hooks.presto import PrestoHook
  File "/usr/local/lib/python3.7/site-packages/airflow/providers/presto/hooks/presto.py", line 31, in <module>
    from airflow.utils.operator_helpers import AIRFLOW_VAR_NAME_FORMAT_MAPPING, DEFAULT_FORMAT_PREFIX
ImportError: cannot import name 'DEFAULT_FORMAT_PREFIX' from 'airflow.utils.operator_helpers' (/usr/local/lib/python3.7/site-packages/airflow/utils/operator_helpers.py)

@potiuk
Copy link
Member

potiuk commented Mar 24, 2022

Jus don't import it from there. or catch an import and provide a suitable default. I do not know the history of DEFAULT_FORMAT_PREFIX so cannot give a definite answer, but definitely if you add this import - you are relying on 2.3 behaviour, which we should not do.
According to https://github.com/apache/airflow/blob/main/README.md#support-for-providers all the providers until 21st of May should be 2.1-compatible.

@potiuk
Copy link
Member

potiuk commented Mar 24, 2022

Seems that it's you who added the prefix, so you should know what to use in the provider to make it 2.1 comatible I think :)

@potiuk
Copy link
Member

potiuk commented Mar 24, 2022

Seems that the prefix was actually added in 2.3 so you MUST make it compatible because 2.2 compatibility will be there for quite a while. The 2.2 compatibility is there till October 11.

@pingzh pingzh force-pushed the pingzh-pass-X-Presto-Client-Info branch from 4ce86ea to 0fb6628 Compare March 24, 2022 16:44
@pingzh pingzh closed this Mar 24, 2022
@pingzh pingzh reopened this Mar 24, 2022
@pingzh pingzh force-pushed the pingzh-pass-X-Presto-Client-Info branch 2 times, most recently from f77da57 to 6ff570f Compare March 24, 2022 19:24
@pingzh pingzh force-pushed the pingzh-pass-X-Presto-Client-Info branch from 6ff570f to 9d18ed9 Compare March 24, 2022 19:25
@potiuk potiuk merged commit 05b4409 into apache:main Mar 24, 2022
@pingzh pingzh deleted the pingzh-pass-X-Presto-Client-Info branch March 24, 2022 21:25
@eladkal
Copy link
Contributor

eladkal commented Mar 24, 2022

@pingzh
Copy link
Contributor Author

pingzh commented Mar 24, 2022

@pingzh Should we do that also for TrinoHook? https://trino.io/docs/current/develop/client-protocol.html#client-request-headers

@eladkal good catch. let me add that as well.

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

Successfully merging this pull request may close these issues.

3 participants