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

Move libjars parameter in Sqoop Hook to Hook parameter #29500

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Feb 12, 2023

The libjars parameter is not needed to be configured in configuration, it should be implemented in Hook (and Operator transitively).


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

The libjars parameter is not needed to be configured in configuration,
it should be implemented in Hook (and Operator transitively).
@@ -61,14 +61,15 @@ def __init__(
hcatalog_database: str | None = None,
hcatalog_table: str | None = None,
properties: dict[str, Any] | None = None,
libjars: str | None = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this idea to extend to files and archives too seeing as they are "not typically used with Sqoop, but they are included as part of Hadoop’s internal argument-parsing system."?

@@ -46,6 +45,7 @@ class SqoopHook(BaseHook):
:param verbose: Set sqoop to verbose.
:param num_mappers: Number of map tasks to import in parallel.
:param properties: Properties to set via the -D argument
:param libjars: Optional Comma separated jar files to include in the classpath.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:param libjars: Optional Comma separated jar files to include in the classpath.
:param libjars: Optional comma separated jar files to include in the classpath.

Nit.

Copy link
Contributor

@josh-fell josh-fell left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Up to you if you feel files and archives parameters should get the same treatment.

@dstandish
Copy link
Contributor

dstandish commented Feb 16, 2023

I don't understand why this change. What's wrong with configuring in connection? Like it's fine to add as hook param but, why not allow continued configuration via airflow connection?

@potiuk
Copy link
Member Author

potiuk commented Feb 16, 2023

I don't understand why this change. What's wrong with configuring in connection? Like it's fine to add as hook param but, why not allow continued configuration via airflow connection?

Just for consistency with all the other similar approaches, any system level configuration like that is better to be controlled by the operator's parameter rather than configuration.

@potiuk potiuk merged commit 655ffb8 into apache:main Feb 16, 2023
38 checks passed
@potiuk potiuk deleted the move-libjars-sqoop-parameter-from-connection-to-hook branch February 16, 2023 13:08
@potiuk
Copy link
Member Author

potiuk commented Feb 16, 2023

Looks fine to me. Up to you if you feel files and archives parameters should get the same treatment.

Yeah. Just a minimum set of changes for this one - we can always separate them outside as new PR.

sirVir pushed a commit to sirVir/airflow that referenced this pull request Mar 14, 2023
The libjars parameter is not needed to be configured in configuration,
it should be implemented in Hook (and Operator transitively).
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.

None yet

3 participants