Skip to content

Apache Pinot provider.yaml references missing PinotHook class #33596

Description

@alexbegg

Apache Airflow version

Other Airflow 2 version (please specify below)

What happened

When starting Airflow (the problem seems to be in both 2.6.3 and in 2.7.0, see "How to reproduce" below) I am getting the following warning:

{providers_manager.py:253} WARNING - Exception when importing 'airflow.providers.apache.pinot.hooks.pinot.PinotHook' from 'apache-airflow-providers-apache-pinot' package
Traceback (most recent call last):
  File "/opt/bitnami/airflow/venv/lib/python3.9/site-packages/airflow/utils/module_loading.py", line 39, in import_string
    return getattr(module, class_name)
AttributeError: module 'airflow.providers.apache.pinot.hooks.pinot' has no attribute 'PinotHook'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/bitnami/airflow/venv/lib/python3.9/site-packages/airflow/providers_manager.py", line 285, in _sanity_check
    imported_class = import_string(class_name)
  File "/opt/bitnami/airflow/venv/lib/python3.9/site-packages/airflow/utils/module_loading.py", line 41, in import_string
    raise ImportError(f'Module "{module_path}" does not define a "{class_name}" attribute/class')
ImportError: Module "airflow.providers.apache.pinot.hooks.pinot" does not define a "PinotHook" attribute/class

I looked into the issue and it appears the problem in the Apache Pinot provider. The airflow/providers/apache/pinot/provider.yaml (which is loaded by the _sanity_check in providers_manager) is referencing a PinotHook class that does not exist:

connection-types:
- hook-class-name: airflow.providers.apache.pinot.hooks.pinot.PinotHook
connection-type: pinot

The module airflow.providers.apache.pinot.hooks.pinot contains PinotAdminHook and PinotDbApiHook, but not PinotHook (and the classes have been separate since before the Apache classes were split into the Apache provider).

I am willing to fix this, but I am not sure which is a better fix:

  1. I could list both classes in connection-types of provider.yaml, but keep both as connection-type: pinot, but there will be two connection types with the same name (which may not be possible?):
    connection-types:
      - hook-class-name: airflow.providers.apache.pinot.hooks.pinot.PinotAdminHook
        connection-type: pinot
      - hook-class-name: airflow.providers.apache.pinot.hooks.pinot.PinotDbApiHook
        connection-type: pinot
    • Note: create_default_connections in airflow/utils/db.py is currently including both connection with the same conn_type="pinot":

      airflow/airflow/utils/db.py

      Lines 474 to 492 in 487b174

      merge_conn(
      Connection(
      conn_id="pinot_admin_default",
      conn_type="pinot",
      host="localhost",
      port=9000,
      ),
      session,
      )
      merge_conn(
      Connection(
      conn_id="pinot_broker_default",
      conn_type="pinot",
      host="localhost",
      port=9000,
      extra='{"endpoint": "/query", "schema": "http"}',
      ),
      session,
      )
  2. or we change one (or both) of the connection types to a different name. PinotAdminHook already uses a default connection name of pinot_admin_default, and PinotDbApiHook already uses a default connection name of pinot_broker_default, so it might make sense to name these connection types pinot_admin and pinot_broker:
    connection-types:
      - hook-class-name: airflow.providers.apache.pinot.hooks.pinot.PinotAdminHook
        connection-type: pinot_admin
      - hook-class-name: airflow.providers.apache.pinot.hooks.pinot.PinotDbApiHook
        connection-type: pinot_broker
    • I think we will need to change create_default_connections in airflow/utils/db.py (as shown above) if we end up changing the connection types. Possibly other places, but I have not seen any other references of conn_type="pinot" beside the default connections.

Thoughts on which approach is better / less disruptive to users?

What you think should happen instead

When starting Airflow the _sanity_check in providers_manager should not trigger a warning. Both connection types should be useable.

How to reproduce

I am seeing this every time I start Airflow with the Docker image bitnami/airflow:2.6.3, but I will also test using Breeze and other ways to see if I see the same warning in 2.7.0. But since the problem line of code is unchanged in the mainbranch I am sure this is still an issue.

Operating System

Debian GNU/Linux 11 (bullseye)

Versions of Apache Airflow Providers

apache-airflow-providers-apache-pinot==4.1.1

Deployment

Docker-Compose

Deployment details

Docker image bitnami/airflow:2.6.3 in Docker Compose (this is the latest Airflow version for Bitnami's image, they have not yet pushed up a 2.7.0 version)

Anything else

The issue #28790 is related as it mentions that both hooks of the Apache Pinot provider is missing conn_type.

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

Activity

  1. Taragolis commented on Aug 21, 2023

    @Taragolis
    Contributor

    I could list both classes in connection-types of provider.yaml, but keep both as connection-type: pinot, but there will be two connection types with the same name (which may not be possible?):

    AFAIK it just took the last one.

    I know we will need to change create_default_connections in airflow/utils/db.py (as shown above) if we end up changing the connection types. Possibly other places, but I have not seen any other references of conn_type="pinot" beside the default connections.

    I'm not sure that we need to change this values, most of them just for demonstrations. And it quite difficult to keep it updated, because "default" connections sits inside of core, and providers might work on different versions of Airflow, for example now min supported version for providers is 2.4

    In additional most of the (if not all) Hooks ignore "connection_type", so if you want to contribute changes to Apache Pinot provider you also could choose name for connection.

    BTW, if you want to work on this issue you also might help in other task and add missing documentation for Apache Pinot, see: #28790

  2. alexbegg commented on Aug 21, 2023

    @alexbegg
    ContributorAuthor

    Thanks, I did see that other issue, as it mentioned this provider.

  3. potiuk commented on Aug 21, 2023

    @potiuk
    Member

    Thoughts on which approach is better / less disruptive to users?

    You should add one with "pinot" and the other with "pinot-admin" connection type. Connection type is only really used when you customize UI (i.e. show some fields for extras or change the hint etc. Connection-type allow you to map the type of connection you choose from UI and which field cusomisations should be happening.

    Pinot does not define any customisations so it does not matter. But anticipating future, it would make sense to have two different connection types for both.

    I'm not sure that we need to change this values, most of them just for demonstrations. And it quite difficult to keep it updated, because "default" connections sits inside of core, and providers might work on different versions of Airflow, for example now min supported version for providers is 2.4

    There is issue to change it #32048 (and it's been followed by Airflow 2.7 changing the commands to create default connections). This is what will be finally moving to providers in 2.8 - similarly as in 2.7 we moved configuration to be part of provider.yaml (or provider_info entrypoint).

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

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions