-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Docs for jdbc interpreter #661
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
Conversation
|
@jongyoul please review and feel free to comment |
docs/interpreter/jdbc.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add this as a code block so that an user can copy/paste it?
ce45c58 to
5f0d120
Compare
|
thanks @felixcheung....It is much better now |
|
thx, you might have forgotten to take out images replaced with text from this PR? |
|
I'm sorry....;) |
|
@vgmartinez thanks, but to clarify, I think it's fine that you have images to show results along with the code that can be pasted. As of now though, you have image files such as jdbc-interpreter-use.png in this PR, but they are not referenced in jdbc.md? I'd suggest removing these from this commit/PR: |
|
ok...it ready now... |
docs/interpreter/jdbc.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry, I've missed this - please update the text as shown in the image since there is no image now?
|
pending some text issues, assuming these are fixed, this looks good. |
|
Done |
|
Hi @vgmartinez, thank you for a new JDBC interpreter and the docs! : ) |
|
Hi @AhyoungRyu....It done |
|
Thanks @vgmartinez for the documentation. |
docs/interpreter/jdbc.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that part really similar to the previous configurations tables?
|
One question about that interpreter, how do you specify the database you want to use? |
|
It depends on the type, but database name is normally part of the jdbc url: https://www.petefreitag.com/articles/jdbc_urls/ maybe we should show some example in the doc. |
|
Yes it would be better to have it documented then. |
|
Hi @corneadoug, |
|
@vgmartinez where is postgres coming from inside |
|
To me there is still plenty of things missing in the doc:
|
|
now I'm busy at work in 1 to 2 days to complete it....thanks for review |
|
No problem |
b3ec52b to
38ccefb
Compare
|
@felixcheung @corneadoug ready to review...I think it is ready... |
|
@vgmartinez I liked the improvements, that was easy to read and self-explanatory. Just a couple of questions about the JDBC interpreter:
|
change bad name in property
|
Hi @corneadoug,
|
|
Awesome! I think its good, waiting for @felixcheung in case he sees something to change |
| <li><a href="{{BASE_PATH}}/interpreter/geode.html">Geode</a></li> | ||
| <li><a href="{{BASE_PATH}}/interpreter/hbase.html">HBase</a></li> | ||
| <li><a href="{{BASE_PATH}}/interpreter/hive.html">Hive</a></li> | ||
| <li><a href="{{BASE_PATH}}/interpreter/jdbc.html">JDBC</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to move this down a line after "Ignite"? (since this is sorted)
|
couple of questions, thanks for adding more details |
|
I fixed @felixcheung... |
docs/interpreter/jdbc.md
Outdated
| </tr> | ||
| <tr> | ||
| <td>default.password</td> | ||
| <td>`********`</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vgmartinez. Thank you for your effort to JDBC docs : )
If you purposed a code block for ********, you have to use <code> tag in the table tag instead of the back quatation mark . Or, it doesn't work.
|
Thanks, please fix and we should merge this unless anyone has more comment. |
|
It's fixed @felixcheung |
|
Thanks could you see this comment #661 (comment) |
|
@felixcheung @AhyoungRyu |
|
@vgmartinez sorry, i think my comment has confused you. |
|
Fix |
|
thanks, merging if no more discussion. |
|
LGTM |
|
LGTM : ) |
Docs for jdbc interpreter Documentation No. But there is related PR: https://issues.apache.org/jira/browse/ZEPPELIN-614 Does the licenses files need update? No Is there breaking changes for older versions? No Does this needs documentation? No Author: Victor Manuel <viktor.manuel.garcia@gmail.com> Author: vgmartinez <viktor.manuel.garcia@gmail.com> Closes apache#661 from vgmartinez/generic_jdbc_docs and squashes the following commits: f9cf476 [Victor Manuel] Fix comment cee96a6 [Victor Manuel] fix docs and add more details cd60020 [Victor Manuel] delete parentheses... d92d7d8 [Victor Manuel] add default prefix in simple connection b973022 [Victor Manuel] change order in link f9f194e [Victor Manuel] Update jdbc.md e3653ba [vgmartinez] fix dead link 38ccefb [vgmartinez] docs for jdbc


What is this PR for?
Docs for jdbc interpreter
What type of PR is it?
Documentation
Todos
Is there a relevant Jira issue?
No. But there is related PR: https://issues.apache.org/jira/browse/ZEPPELIN-614
How should this be tested?
Screenshots (if appropriate)
Questions:
Does the licenses files need update? No
Is there breaking changes for older versions? No
Does this needs documentation? No