Skip to content

Remove unnecessary WARNINGs from BeamHook #14554

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.

Sign up for GitHub

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

Jump to bottom
Merged
potiuk merged 1 commit into from
Mar 2, 2021
Merged

Remove unnecessary WARNINGs from BeamHook #14554

potiuk merged 1 commit into from
Mar 2, 2021

Conversation

@xinbinhuang
Copy link
Contributor

@xinbinhuang xinbinhuang commented Mar 2, 2021 •
edited
Loading

BeamHook is not discoverable so it shouldn't be added under hook-class-names in providers.yaml.

Currently, it's throwing quite a lot of warnings on startup or anytime you need to do ProvidersManagers().

WARNI [airflow.providers_manager] The '<class 'airflow.providers.apache.beam.hooks.beam.BeamHook'>' is missing conn_type attribute and cannot be registered
WARNI [airflow.providers_manager] The '<class 'airflow.providers.apache.beam.hooks.beam.BeamHook'>' is missing conn_name_attr attribute and cannot be registered
WARNI [airflow.providers_manager] The '<class 'airflow.providers.apache.beam.hooks.beam.BeamHook'>' is missing hook_name attribute and cannot be registered
WARNI [airflow.providers_manager] The '<class 'airflow.providers.apache.beam.hooks.beam.BeamHook'>' is missing conn_type attribute and cannot be registered
WARNI [airflow.providers_manager] The '<class 'airflow.providers.apache.beam.hooks.beam.BeamHook'>' is missing conn_name_attr attribute and cannot be registered
WARNI [airflow.providers_manager] The '<class 'airflow.providers.apache.beam.hooks.beam.BeamHook'>' is missing hook_name attribute and cannot be registered

cc: @TobKed @potiuk

^ 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.

@xinbinhuang xinbinhuang mentioned this pull request Mar 2, 2021
Closed
30 tasks
@xinbinhuang xinbinhuang changed the title Remove WARNINGs from BeamHook Remove unnecessary WARNINGs from BeamHook Mar 2, 2021
@turbaszek turbaszek mentioned this pull request Mar 2, 2021
Merged
@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 2, 2021
@github-actions
Copy link

github-actions bot commented Mar 2, 2021

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 master or amend the last commit of the PR, and push it with --force-with-lease.

RosterIn
RosterIn reviewed Mar 2, 2021
View reviewed changes
TobKed
TobKed suggested changes Mar 2, 2021
View reviewed changes
airflow/providers/apache/beam/provider.yaml
Comment on lines -44 to -46

hook-class-names:
- airflow.providers.apache.beam.hooks.beam.BeamHook
Copy link
Contributor

@TobKed TobKed Mar 2, 2021

Choose a reason for hiding this comment

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

Is it not better to add conn_type, conn_name_attr and hook_name to BeamHook instead of deleting these lines?
(e.g in Casssanda:

airflow/airflow/providers/apache/cassandra/hooks/cassandra.py

Line 86 in b34f236

conn_name_attr = 'cassandra_conn_id'
)

Copy link
Contributor Author

@xinbinhuang xinbinhuang Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's a bit tricky for BeamHook to have those attributes because Beam doesn't tie to a specified "backend/runner", it can be run on Dataflow, Flink, Spark, etc. and so their credentials may also come in different forms (i.e. the current operator is using DataFlow).
Overall, I think it's possible to specify those attributes, but it requires some extra thoughts on what the Connection object for Beam will look like. For now, we can at least get rid of those warnings whenever ProvidersManager is instantiated, and this is the intention of this PR.

Copy link
Member

@potiuk potiuk Mar 2, 2021

Choose a reason for hiding this comment

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

Agree. It is better just to remove it for now.

@potiuk potiuk merged commit 4e57630 into apache:master Mar 2, 2021
@TobKed TobKed mentioned this pull request Mar 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@turbaszek turbaszek turbaszek approved these changes

@potiuk potiuk potiuk approved these changes

+2 more reviewers

@RosterIn RosterIn RosterIn left review comments

@TobKed TobKed TobKed requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@xinbinhuang @potiuk @turbaszek @TobKed @RosterIn