Skip to content

Extend jdbc functionality - #185

Merged
mistercrunch merged 8 commits into
apache:masterfrom
wooga:af-jdbc
Jul 28, 2015
Merged

Extend jdbc functionality#185
mistercrunch merged 8 commits into
apache:masterfrom
wooga:af-jdbc

Conversation

@jomar83

@jomar83 jomar83 commented Jul 26, 2015

Copy link
Copy Markdown
Contributor
  • Adjust web ui to allow definition of jdbc connections
  • Use host column for jdbc connection url
  • Use connection models extra for storing jdbc driver path and classname
  • Use connections extra_dejson to create the jdbc connection
  • Use DbApiHook as a base for JdbcHook
  • Make JdbcOperator consistent with other Db ops by using run

@jomar83

jomar83 commented Jul 26, 2015

Copy link
Copy Markdown
Contributor Author

Example of creating a connection:

create jdbc connection

@jomar83

jomar83 commented Jul 27, 2015

Copy link
Copy Markdown
Contributor Author

Maybe a little clarification on this. The reason we extended this further was due to the fact, that adhoc queries to jdbc wouldn't work. In short, predefining jdbc connections wasn't possible, because there was no way to define jdbc driver location and jdbc driver classname. Now that's possible and everything works including adhoc queries.

Comment thread airflow/hooks/jdbc_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.

I think there's no need for an __init__ anymore as DbApiHook should work just fine.

You can move the get_connection call into get_conn and pass the info straight into the connect call.

@mistercrunch

Copy link
Copy Markdown
Member

About the UI customization, it's a nice flask-admin cutomization that optimizes for user experience but moves away from vanilla CRUD. I'm not sure if I want to optimize for user experience or ease of maintenance though. We could just document how to use the extras key/values very clearly and get rid of a bunch jdbc custom logic that requires knowledge of flask_admin/javascript/jinja to understand/evolve.

On the other hand it looks really nice and fits the JDBC use case.

What do you think?

@jomar83

jomar83 commented Jul 27, 2015

Copy link
Copy Markdown
Contributor Author

I think it's a difficult decision.

That said, if you want to get as many people/companies to use airflow as possible, it's probably worth doing a little UI customisation to get a bigger audience to use it. Usability will go a long way when it comes to that. Good documentation will mitigate this to a certain extend, but I think overall the changes that were necessary weren't that intrusive. Still, your argument is valid and it's a tradeoff worth discussing. Let's see if someone else has an opinion on the matter.

In the meantime I'll fix the other things.

@tkaymak

tkaymak commented Jul 28, 2015

Copy link
Copy Markdown
Contributor

The design is based on the goal of making the setup of JDBC Drivers as simple as possible (via the webinterface and not by diving into the code).

@mistercrunch

Copy link
Copy Markdown
Member

Totally, thanks for taking the time to bend the form into doing this, it also sets a precedent on how to do it if we want to replicate it for other hooks/connections.

@mistercrunch

Copy link
Copy Markdown
Member

img

mistercrunch added a commit that referenced this pull request Jul 28, 2015
Extend jdbc functionality
@mistercrunch
mistercrunch merged commit 22c8f21 into apache:master Jul 28, 2015
@mistercrunch

Copy link
Copy Markdown
Member

Fixing a bug and generalizing the idea here:
#201

@m1racoli
m1racoli deleted the af-jdbc branch August 28, 2015 12:18
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.

3 participants