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.

Sign up for GitHub

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

Jump to bottom

[ZEPPELIN-1366] Removed legacy JDBC alias #1360

Closed
wants to merge 5 commits into from
Closed

[ZEPPELIN-1366] Removed legacy JDBC alias #1360

wants to merge 5 commits into from

Conversation

jongyoul
Copy link
Member

What is this PR for?

Removing old JDBC sugar

What type of PR is it?

[Feature]

Todos

  • - Removed codes to check jdbc alias enabled

What is the Jira issue?

How should this be tested?

  • No longer available
    • %jdbc(mysql) -> %mysql

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? Yes
  • Does this needs documentation? Yes, but I don't know where the proper location is.

@bzz
Copy link
Member

bzz commented Aug 26, 2016 •
edited

LGTM. But this needs to be rebased on latest master - seems like there is a conflict now.

Is there breaking changes for older versions? Yes

Do we have a change-log or something where we can keep track of breaking changes to highlight them in the release?

Does this needs documentation? Yes, but I don't know where the proper location is.

@minahlee @AhyoungRyu what do you think is the best location for such docs?

@Leemoonsoo
Copy link
Member

I think it changes way of configuration. From single jdbc interpreter setting manages multiple jdbc connection, to individual interpreter setting manage single jdbc connection.

How about remove non-default jdbc connection configurations from default properties and documentation?
https://github.com/apache/zeppelin/blob/master/jdbc/src/main/resources/interpreter-setting.json
https://github.com/apache/zeppelin/blob/master/docs/interpreter/jdbc.md

And incompatible changes can be guided in https://github.com/apache/zeppelin/blob/master/docs/install/upgrade.md

@jongyoul
Copy link
Member Author

@bzz @Leemoonsoo Thanks, I don't know the proper location to describe the changes actually. I'll also update upgrade.md. And about changing a behaviour of JDBC, in case of %jdbc(mysql), I thought it's not related to this issue directly, then I'll make another PR for handling that. We don't need to multiple configurations due to interpreter aliases.

@AhyoungRyu
Copy link
Contributor

@jongyoul Then can we use JDBC interpreter as described in How to use section like before? I just wondered :D

@jongyoul
Copy link
Member Author

jongyoul commented Aug 29, 2016 •
edited

@AhyoungRyu Sure, That works for now. I'll remove those docs after I remove the prefix feature.

@jongyoul
Copy link
Member Author

Finished rebase. Ci failure is unrelated. Merging if there's no more discussion.

Leemoonsoo
Leemoonsoo reviewed Aug 30, 2016
View reviewed changes
docs/install/upgrade.md
# Breaking Changes
## Upgrading from 0.6.x to 0.7.0
- Mapping from `%jdbc(prefix)` to `%prefix` is no longer available. You can set multiple interpreters with same interpreter group.
Copy link
Member

Choose a reason for hiding this comment

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

nit: How about "Instead, you can use %[interpreter alias] with multiple interpreter setttings on GUI", instead of "You can set multiple interpreters with same interpreter group"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that looks more kind

@jongyoul
Copy link
Member Author

Addressed upgrade.md

@Leemoonsoo
Copy link
Member

Thanks LGTM

@jongyoul
Copy link
Member Author

jongyoul commented Sep 2, 2016

I've rebased it because of resolving conflict. I'll merge it soon.

@asfgit asfgit closed this in fe3dbdb Sep 2, 2016
@AhyoungRyu AhyoungRyu mentioned this pull request Oct 3, 2016
Closed
asfgit pushed a commit that referenced this pull request Oct 11, 2016
@AhyoungRyu @Leemoonsoo
[ZEPPELIN-1489] Remove jdbc-like interpreter example properties and i…
33677c0 
…mprove JDBC docs

### What is this PR for?
Currently we can use `%jdbc(prefix)` for multi database connections(e.g. configuring both postgresql and hive in one JDBC interpreter). But after #1360 merged, Zeppelin doesn't support `%jdbc(prefix)` to `%prefix` anymore. So auto-prefix feature is not working for `%jdbc(prefix)`. The auto-prefix only works for `%prefix` now. Considering this status, it would be better we remove the JDBC connection examples in `jdbc/interpreter-setting.json` (this is come from #1096) so that users can create several JDBC interpreters instead of using multiple connections with one JDBC interpreter.  I removed the related contents in `jdbc.md` as well.

Also, the contents of [current JDBC documentation page](http://zeppelin.apache.org/docs/0.7.0-SNAPSHOT/interpreter/jdbc.html) is quite confusing and unorganized. So I updated the contents with some screenshot images to guide "How to create JDBC interpreter", "How to edit the interpreter properties for the connection", "How to use `%prefix` with the interpreter", and so on.

### What type of PR is it?
Improvement & Documentation

### What is the Jira issue?
[ZEPPELIN-1489](https://issues.apache.org/jira/browse/ZEPPELIN-1489)

### How should this be tested?
* Removing example properties in `interpreter-setting.json`
  1. after applying this patch and build with `mvn clean package -DskipTests -Pspark-1.6 -pl 'jdbc,zeppelin-interpreter,zeppelin-web,zeppelin-server,zeppelin-zengine,zeppelin-display'`
  2. create JDBC interpreter and check whether the example settings are gone or not

* JDBC docs
  1. Build only `docs/` dir as described in [here](https://github.com/apache/zeppelin/blob/master/docs/README.md#build-documentation)
  2. Go to `interpreter -> JDBC` and just compare this locally builded page with [the original JDBC page](https://zeppelin.apache.org/docs/0.7.0-SNAPSHOT/interpreter/jdbc.html)

### Screenshots (if appropriate)
 - Before
<img width="1579" alt="before" src="https://cloud.githubusercontent.com/assets/10060731/19045323/1ff0c706-89d3-11e6-9b6f-dc75877f81f3.png">

 - After
<img width="944" alt="screen shot 2016-10-04 at 1 05 00 am" src="https://cloud.githubusercontent.com/assets/10060731/19045324/24a9187a-89d3-11e6-90d6-b80acbc6af7c.png">

 - Some parts of updated JDBC docs
Since many contents are changed, it would be better to build `docs/` locally to review all of the change.

<img width="695" alt="screen shot 2016-10-04 at 12 36 39 am" src="https://cloud.githubusercontent.com/assets/10060731/19043794/9d9a32fc-89cc-11e6-9d15-f6036a1b738e.png">

<img width="704" alt="screen shot 2016-10-04 at 12 36 59 am" src="https://cloud.githubusercontent.com/assets/10060731/19043800/a62fc90e-89cc-11e6-976d-5c697729eca4.png">

<img width="677" alt="screen shot 2016-10-04 at 12 37 31 am" src="https://cloud.githubusercontent.com/assets/10060731/19043807/acbc9766-89cc-11e6-8c73-eab1cc18440b.png">

<img width="688" alt="screen shot 2016-10-04 at 12 37 43 am" src="https://cloud.githubusercontent.com/assets/10060731/19043816/b06b5690-89cc-11e6-9298-a20b49fea622.png">

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? yes

Author: AhyoungRyu <ahyoungryu@apache.org>

Closes #1480 from AhyoungRyu/ZEPPELIN-1489 and squashes the following commits:

76bf55e [AhyoungRyu] Minor update
d5aaa97 [AhyoungRyu] Remove useless screenshot images
c6f9ed4 [AhyoungRyu] Add screenshot images
99a18e2 [AhyoungRyu] Remove jdbc setting examples in interpreter-setting.json
050ecc0 [AhyoungRyu] Update jdbc.md
20da102 [AhyoungRyu] Redshit -> Redshift in JDBCInterpreter.java
pedrozatta pushed a commit to pedrozatta/zeppelin that referenced this pull request Oct 27, 2016
@AhyoungRyu
[ZEPPELIN-1489] Remove jdbc-like interpreter example properties and i…
6c7a744 
…mprove JDBC docs

### What is this PR for?
Currently we can use `%jdbc(prefix)` for multi database connections(e.g. configuring both postgresql and hive in one JDBC interpreter). But after apache#1360 merged, Zeppelin doesn't support `%jdbc(prefix)` to `%prefix` anymore. So auto-prefix feature is not working for `%jdbc(prefix)`. The auto-prefix only works for `%prefix` now. Considering this status, it would be better we remove the JDBC connection examples in `jdbc/interpreter-setting.json` (this is come from apache#1096) so that users can create several JDBC interpreters instead of using multiple connections with one JDBC interpreter.  I removed the related contents in `jdbc.md` as well.

Also, the contents of [current JDBC documentation page](http://zeppelin.apache.org/docs/0.7.0-SNAPSHOT/interpreter/jdbc.html) is quite confusing and unorganized. So I updated the contents with some screenshot images to guide "How to create JDBC interpreter", "How to edit the interpreter properties for the connection", "How to use `%prefix` with the interpreter", and so on.

### What type of PR is it?
Improvement & Documentation

### What is the Jira issue?
[ZEPPELIN-1489](https://issues.apache.org/jira/browse/ZEPPELIN-1489)

### How should this be tested?
* Removing example properties in `interpreter-setting.json`
  1. after applying this patch and build with `mvn clean package -DskipTests -Pspark-1.6 -pl 'jdbc,zeppelin-interpreter,zeppelin-web,zeppelin-server,zeppelin-zengine,zeppelin-display'`
  2. create JDBC interpreter and check whether the example settings are gone or not

* JDBC docs
  1. Build only `docs/` dir as described in [here](https://github.com/apache/zeppelin/blob/master/docs/README.md#build-documentation)
  2. Go to `interpreter -> JDBC` and just compare this locally builded page with [the original JDBC page](https://zeppelin.apache.org/docs/0.7.0-SNAPSHOT/interpreter/jdbc.html)

### Screenshots (if appropriate)
 - Before
<img width="1579" alt="before" src="https://cloud.githubusercontent.com/assets/10060731/19045323/1ff0c706-89d3-11e6-9b6f-dc75877f81f3.png">

 - After
<img width="944" alt="screen shot 2016-10-04 at 1 05 00 am" src="https://cloud.githubusercontent.com/assets/10060731/19045324/24a9187a-89d3-11e6-90d6-b80acbc6af7c.png">

 - Some parts of updated JDBC docs
Since many contents are changed, it would be better to build `docs/` locally to review all of the change.

<img width="695" alt="screen shot 2016-10-04 at 12 36 39 am" src="https://cloud.githubusercontent.com/assets/10060731/19043794/9d9a32fc-89cc-11e6-9d15-f6036a1b738e.png">

<img width="704" alt="screen shot 2016-10-04 at 12 36 59 am" src="https://cloud.githubusercontent.com/assets/10060731/19043800/a62fc90e-89cc-11e6-976d-5c697729eca4.png">

<img width="677" alt="screen shot 2016-10-04 at 12 37 31 am" src="https://cloud.githubusercontent.com/assets/10060731/19043807/acbc9766-89cc-11e6-8c73-eab1cc18440b.png">

<img width="688" alt="screen shot 2016-10-04 at 12 37 43 am" src="https://cloud.githubusercontent.com/assets/10060731/19043816/b06b5690-89cc-11e6-9298-a20b49fea622.png">

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? yes

Author: AhyoungRyu <ahyoungryu@apache.org>

Closes apache#1480 from AhyoungRyu/ZEPPELIN-1489 and squashes the following commits:

76bf55e [AhyoungRyu] Minor update
d5aaa97 [AhyoungRyu] Remove useless screenshot images
c6f9ed4 [AhyoungRyu] Add screenshot images
99a18e2 [AhyoungRyu] Remove jdbc setting examples in interpreter-setting.json
050ecc0 [AhyoungRyu] Update jdbc.md
20da102 [AhyoungRyu] Redshit -> Redshift in JDBCInterpreter.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@jongyoul @bzz @Leemoonsoo @AhyoungRyu