Skip to content

Add JDBC Interpreter for Mysql - #60

Closed
1teed wants to merge 3 commits into
apache:masterfrom
1teed:master
Closed

Add JDBC Interpreter for Mysql#60
1teed wants to merge 3 commits into
apache:masterfrom
1teed:master

Conversation

@1teed

@1teed1teed commented May 4, 2015

Copy link
Copy Markdown
Contributor

I added jdbc interpreter for mysql

I used Abstract Factory Pattern
It has the advantage that can be separated using the JDBC Connection only argument value

1

How to add your jdbc driver

  1. add dependency for your jdbc driver (pom.xml)
  2. implement Sub-Class (like MysqlConnection)
  3. add branch connection in Factory Class (DBConnectionFactory)

It can be extended any jdbc. Such as Oracle, HIVE, Tajo, hBase, MariaDB, ...

Set interpreter
2015-05-04 18 47 59

How to Use
2015-05-04 19 02 27

@Leemoonsoo

Copy link
Copy Markdown
Member

Thanks for the contribution.

For default value of zeppelin-site.xml, you'll need add JDBCInterpreter to zeppelin-zengine/src/main/java/org/apache/conf/ZeppelinConfiguration.java L384, ZEPPELIN_INTERPRETERS.

And about mysql dependency in jdbc/pom.xml file,

 <dependency>
<groupId>mysql</groupId>
<artifactId>mysql-connector-java</artifactId>
<version>5.1.6</version>
</dependency>

I think the dependency's version is The GNU General Public License, Version 2 and should not be used.

hyonaldo added 2 commits May 6, 2015 19:05
…main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java
@1teed

1teed commented May 6, 2015

Copy link
Copy Markdown
ContributorAuthor

@Leemoonsoo

Thanks for your comment.
I fix mysql dependency to meet The GNU General Public License, Version 2 in pom.xml.
And add JDBCInterpreter to ZEPPELIN_INTERPRETERS in zeppelin-zengine/src/main/java/org/apache/conf/ZeppelinConfiguration.java.

@sylvinho81

Copy link
Copy Markdown

Hi @1teed,

I am trying to apply this patch and test it, because I think is really interesting and I need it.
After apply the changes and run:
mvn clean package
mvn install -DskipTests -Dspark.version=1.1.1 -Dhadoop.version=1.2.1

I can see:

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] Zeppelin ........................................... SUCCESS [ 3.472 s]
[INFO] Zeppelin: Interpreter .............................. SUCCESS [ 9.269 s]
[INFO] Zeppelin: Zengine .................................. SUCCESS [ 3.835 s]
[INFO] Zeppelin: Spark .................................... SUCCESS [ 23.308 s]
[INFO] Zeppelin: Markdown interpreter ..................... SUCCESS [ 2.025 s]
[INFO] Zeppelin: Angular interpreter ...................... SUCCESS [ 1.958 s]
[INFO] Zeppelin: Shell interpreter ........................ SUCCESS [ 1.993 s]
[INFO] Zeppelin: Hive interpreter ......................... SUCCESS [ 2.094 s]
[INFO] Zeppelin: Tajo interpreter ......................... SUCCESS [ 1.971 s]
[INFO] Zeppelin: JDBC interpreter ......................... SUCCESS [ 1.922 s]
[INFO] Zeppelin: web Application .......................... SUCCESS [ 45.557 s]
[INFO] Zeppelin: Server ................................... SUCCESS [ 4.682 s]
[INFO] Zeppelin: Packaging distribution ................... SUCCESS [ 0.439 s]

But when I execute "bin/zeppelin-daemon.sh start" and try to add the interpreter in http://localhost:8080/ I can not see the option "jdbc" in the dropdown "Interpreter".

@1teed

1teed commented May 7, 2015

Copy link
Copy Markdown
ContributorAuthor

@sylvinho81

Thank you for your interest.

I think you seem to be not latest source.
Please update to the latest sources.

Please check this update.
2015-05-07 14 59 15

@sylvinho81

Copy link
Copy Markdown

Thanks @1teed. I had not seen the latest changes

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.

Since this is using java.sql.DriverManager, it should support all JDBC driver with the same code, and not just mysql, correct?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@felixcheung are correct.
Using java.sql.DriverManager is support all JDBC driver with the same code.
(such as jdbc/src/main/java/org/apache/zeppelin/jdbc/MysqlConnection.java : Line 73)

But every JDBC driver name and url pattern is different.

@jmrr

jmrr commented May 12, 2015

Copy link
Copy Markdown

@1teed Thanks for the contribution. Tested with a centos-zeppeling docker image and works fine! Looking forward to the merge!

Comment threadjdbc/pom.xml

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.

version should be 0.5.0-incubating-SNAPSHOT
and line 31, too. (check #59)

@Leemoonsoo

Copy link
Copy Markdown
Member

@hyonaldobe4b52b changes the mysql dependency version. but it is still GPLv2 License.

In my understanding, If code is depends on some GPL (depends means more like can not functioning without it), it can not be included.
And in our case, mysql driver is GPL and this interpreter looks like pretty much depends on mysql driver.

If i'm right, I think it's better to make JdbcInterpreter completely independent to mysql.
And then we can make something similar to dependency loader of sparkinterpreter. so any jdbc driver could be loaded on the fly Instead of adding them in pom.xml.

Any ideas?

@jmrr

jmrr commented May 21, 2015

Copy link
Copy Markdown

Are temporary tables possible with the interpreter connector? I quickly tried one and got Can not issue data manipulation statements with executeQuery().

Apart from that everything else runs smoothly.

Thanks!

@ZhijieWang

Copy link
Copy Markdown

@1teed I added postgresql support and create a merge request to your repo.

Limitation of this interpreter -- may crush zeppelin when running large query.

@jongyoul

Copy link
Copy Markdown
Member

@Leemoonsoo Concerning licenses, I've heard that GPL shouldn't be included in apache projects, and known that some project in Apache also doesn't have a mysql-connector-java packages due to same reason. I think JdbcInterpreter looks better and general. @1teed How about you?

@riomario1

Copy link
Copy Markdown

Looks good, when are we merging it?

@jmrr

jmrr commented Jun 10, 2015

Copy link
Copy Markdown

+1 please go ahead and merge into master

On Wed, Jun 10, 2015 at 6:40 PM, riomario1 notifications@github.com wrote:

Looks good, when are we merging it?


Reply to this email directly or view it on GitHub
#60 (comment)
.

@jmrr

jmrr commented Jul 2, 2015

Copy link
Copy Markdown

What happened to this? It hasn't been merged to master yet and I'm having problems merging it locally as the master branch is starting to diverge...

Thanks

@rquast

Copy link
Copy Markdown

Is this getting merged still? Just tried it out and found that it could do with some default settings for the interpreter (jdbc - default it to mysql, host, user, password, port). Works great - very useful.

@Leemoonsoo

Copy link
Copy Markdown
Member

I'm not sure about merging it, while this implementation depends on mysql-connector-java that distributed in GPL 2 license. Please correct me if i'm wrong.

@tdunning

Copy link
Copy Markdown

Yes. The GPL license makes mysql-connector be category X. We cannot ship a release with that component, even unmodified.

@martin-g

Copy link
Copy Markdown
Member

Do you pack mysql-connector.jar within the release?
If YES then there is a problem.
If the user has to provide the jar herself (e.g. by executing mvn install locally) then everything is fine.

@riomario1

Copy link
Copy Markdown

Hi,

How do I use MySQL with zeppelin?

Please provide steps

Thank you

@Leemoonsoo

Copy link
Copy Markdown
Member

Do you pack mysql-connector.jar within the release?

I think answer currently is yes, although binary packages are provided as a favor.

Exclude mysql interpreter by default from the build, but let user enabling with some special flag, will this solve the license issue?

@martin-g

Copy link
Copy Markdown
Member

At http://apache.cbox.biz/incubator/zeppelin/0.5.0-incubating/ (my neareast Apache mirror) I see two binary downloads and one source download.
If you don't include mysql-connector.jar in the binaries then everything is fine from legal point of view.
People wanting to use the MySQL interpreter should build Zeppelin from source (i.e. should use the third download: zeppelin-0.5.0-incubating.tgz), possible with some Maven profile that enables/activates MySQL interpreter.
Zeppelin's distributions should not bring mysql-connector.jar with them. Maven's <dependency>...</dependency> and code like import com.mysql.* doesn't violate the rules, so it is safe to have it in Apache sources.

@neptun2000

Copy link
Copy Markdown

When do you planning to release MySQL interpreter ?
Thanks

@riomario1

Copy link
Copy Markdown

Mysql is the most awaited feature :)

Sent from my iPhone

On Aug 29, 2015, at 4:18 PM, neptun2000 notifications@github.com wrote:

When do you planning to release MySQL interpreter ?
Thanks


Reply to this email directly or view it on GitHub.

@felixcheungfelixcheung mentioned this pull request Oct 25, 2015
@jongyoul

Copy link
Copy Markdown
Member

@1teed Hi, do you still have an interest to contribute this feature? I've found that your PR is the first one for supporting JDBC thus I think it's important to ask your opinion.

@jongyoul

Copy link
Copy Markdown
Member

@1teed Now, I will try to contact @andrescelis to accept jdbc interpreter. I hope you would understand it. And please check the mailing(https://www.mail-archive.com/dev@zeppelin.incubator.apache.org/msg05504.html) and issue(https://issues.apache.org/jira/browse/ZEPPELIN-439)

@jongyoul

Copy link
Copy Markdown
Member

@1teed please close this PR.

Sign up for freeto 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.

13 participants

@1teed@Leemoonsoo@sylvinho81@jmrr@ZhijieWang@jongyoul@riomario1@rquast@tdunning@martin-g@neptun2000@felixcheung@hyonaldo