Skip to content

[AIRFLOW-5529] support apache drill hook#6234

Closed
blcksrx wants to merge 1 commit into
apache:masterfrom
blcksrx:drill
Closed

[AIRFLOW-5529] support apache drill hook#6234
blcksrx wants to merge 1 commit into
apache:masterfrom
blcksrx:drill

Conversation

@blcksrx
Copy link
Copy Markdown
Contributor

@blcksrx blcksrx commented Oct 2, 2019

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
    • https://issues.apache.org/jira/browse/AIRFLOW-XXX
    • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
    • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
    • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

Support Apache Drill hook

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Copy link
Copy Markdown
Member

@XD-DENG XD-DENG left a comment

Choose a reason for hiding this comment

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

My 2 cents

Comment thread setup.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure if it's a good idea to introduce dependency in such a way.

Copy link
Copy Markdown
Contributor Author

@blcksrx blcksrx Oct 2, 2019

Choose a reason for hiding this comment

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

I need to use this repository, Unfortunately, this repository does not exist on PYPI packages repository

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then this PR can't be accepted in to Airflow.

Copy link
Copy Markdown
Contributor Author

@blcksrx blcksrx Oct 2, 2019

Choose a reason for hiding this comment

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

Because of using none PIPY package?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correct, yeah.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is to ensure that each release/version can run stably & would not be broken because of dependency.

Believe you can understand @blcksrx . I would suggest to close this PR first & re-open it once you address this essential 'issue'. Thanks for the contribution though, and we would be happy to take another look when you make it ready.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are Right. Unfortunately, the repo is not active and I' will close it. thanks.

Comment thread airflow/utils/db.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why two connections with the same id are merged here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, Sorry. I will fix it soon.

Comment thread airflow/hooks/drill_hook.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Such a docstring may not be info-rich enough and seems typo inside.

Copy link
Copy Markdown
Contributor Author

@blcksrx blcksrx Oct 2, 2019

Choose a reason for hiding this comment

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

I will improve it but I did write it by comparing with presto_hook. I will add example on it.

@blcksrx blcksrx requested a review from XD-DENG October 2, 2019 11:26
@blcksrx
Copy link
Copy Markdown
Contributor Author

blcksrx commented Oct 2, 2019

@XD-DENG I didn't get why it should failed on flake8 and pylint

@blcksrx blcksrx closed this Oct 2, 2019
@OmerJog
Copy link
Copy Markdown
Contributor

OmerJog commented Oct 28, 2019

@blcksrx it failed because:
airflow/hooks/drill_hook.py:39:39: E999 SyntaxError: invalid syntax

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants