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

Adding template support in qbol operator #1090

Merged
merged 3 commits into from
Mar 4, 2016

Conversation

msumit
Copy link
Contributor

@msumit msumit commented Feb 26, 2016

No description provided.

@@ -2,7 +2,6 @@
from airflow.utils import apply_defaults
from airflow.contrib.hooks import QuboleHook


Copy link
Member

Choose a reason for hiding this comment

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

PEP8 says 2 lines before class definitions

@mistercrunch
Copy link
Member

I feel like all these attributes aren't guaranteed to be present and that the framework will choke trying to template missing attributes. The contract of what one should provide is unclear.

If they are required, they should be explicitly defined in the constructor header (with defaults), and stamped into self in init.py.

@mistercrunch
Copy link
Member

I read the way things are organized in the hook and operator and it's a bit messy the way kwargs is passed around. I saw how you overrode the get_attriubte is a bit hacky too. The operator doesn't have a good doc string to compensate for this behavior... Consider making a clearer interface to the operator, or documenting its expected behavior in the doc string.

@msumit
Copy link
Contributor Author

msumit commented Feb 29, 2016

Hi Maxime,

I do agree with your concerns. Actually we didn't wanted to add dozens of new operators like QuboleHiveOperator, QuboleHadoopOperator etc, so we'd to do hacks like that. If we had individual command operators like I said above, then we could adhere to simple design like the other operators have.

So either I can try to write down the behaviour in more better way, or you can suggest me some alternative approach for supporting templates?

Thanks,
Sumit

@mistercrunch
Copy link
Member

👍

mistercrunch added a commit that referenced this pull request Mar 4, 2016
Adding template support in qbol operator
@mistercrunch mistercrunch merged commit f2196dd into apache:master Mar 4, 2016
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.

2 participants