[ZEPPELIN-1614] Remove markdown4j dep #1594

1ambda · 2016-11-04T04:07:18Z

What is this PR for?

Remove unmanaged, old library markdown4j dep which exists just for Websequence and YUML plugins.
(related to #1384)

By adding Websequence and YUML plugins to pegdown

Removing markdown4j dependency which is unmanaged library currently.
Addtionally, we can remove markdown.parser.type options in markdown interpreter
Fixing some bugs in Websequence and YUML plugins
Enable others to add more plugins using pegdown sytnax.

What type of PR is it?

Improvement

Todos

Nothing

What is the Jira issue?

JIRA - ZEPPELIN-1614

How should this be tested?

Some functional tests are included.

Screenshots (if appropriate)

Questions:

Does the licenses files need update? - YES
Is there breaking changes for older versions? - NO
Does this needs documentation? - YES

bzz · 2016-11-04T04:41:56Z

markdown/src/main/java/org/apache/zeppelin/markdown/PegdownWebSequencelPlugin.java

+  }
+
+  public PegdownWebSequencelPlugin(Integer options,
+                                   Long maxParsingTimeInMillis,


In general, Zeppelin project Jave style guide discourage using of horizontal alignment

bzz · 2016-11-04T04:42:55Z

markdown/src/main/java/org/apache/zeppelin/markdown/PegdownWebSequencelPlugin.java

+        return "http://www.websequencediagrams.com/" + json.substring(start, end);
+      }
+    } catch (IOException e) {
+      throw new RuntimeException("Failed to get proper response from websequencediagrams.com");


It's always good idea to propagate e as cause though Constructor to aid debugging.

bzz · 2016-11-04T04:44:11Z

markdown/src/main/java/org/apache/zeppelin/markdown/PegdownWebSequencelPlugin.java

+      int end = json.indexOf("\"", start);
+
+      if (start != -1 && end != -1) {
+        return "http://www.websequencediagrams.com/" + json.substring(start, end);


Is there a reason why url string, hardcoded in multiple places, can not be replaced with the constant field?

bzz · 2016-11-04T04:46:34Z

Websequence are awesome, thank you @1ambda for contributing this simplification of MD interpreter.

Looks good to me, modulo few minor issues, commented above.
\cc @felixcheung as I remeber he was a user of YUML and Websequence

As soon as CI is green and issues addressed - will be happy to merge to master, in case there is no further discussion.

felixcheung · 2016-11-04T04:59:11Z

docs/interpreter/markdown.md

+<img src="../assets/themes/zeppelin/img/docs-img/markdown-example-github-flavored-parser.png" width="70%" />

-<img src="../assets/themes/zeppelin/img/docs-img/markdown-example-markdown4j-parser.png" width="70%" />
+### Supproted Plugins 


"Supported"?

felixcheung · 2016-11-04T05:05:56Z

This is awesome, thanks!
+1 on @bzz 's comments

1ambda · 2016-11-04T06:30:36Z

Thanks for the review @bzz, @felixcheung! I'v just fixed :)

Leemoonsoo · 2016-11-06T20:45:53Z

Thanks @1ambda for the contribution.
I have compared output from markdown4j and pegdown and found some differences.

font difference

emphasis inside of ()

There could be more unknown differences. That means, existing notebook might render differently with pegdown and that could be unexpected result in some cases.

So how about make pegdown default parser but still keep markdown4j parser support? That at least give user option to configure markdown interpreter with old markdown4j parser when they don't want to make change on existing notebook.

Leemoonsoo · 2016-11-06T20:52:14Z

And i think it'll be better mention in docs/install/upgrade.html#upgrading-from-zeppelin-06-to-07 about default markdown parser change and result slightly different output rendering of markdown paragraphs. And link to markdown parser configuration section in the doc would help as well.

1ambda · 2016-11-07T05:18:20Z

@Leemoonsoo Thanks for review 👍

It would be better to

Use pegdown as default
Add docs for it
Revert markdown4j

Regarding to rendered output you provided, I think it is a bug of pegdown.
But even if it is resolved, there might be differences we can miss. :(
So keep markdown4j. (I overlooked backward compatibility issue)

bzz · 2016-11-09T04:02:54Z

What should be the next steps here @1ambda?

1ambda · 2016-11-09T09:52:43Z

@bzz Resolving CI failure will be next step. But i think it is not problem of the markdown processor. All tests are failed in scalding interpreter test

I will rebase and retrigger CI again.

// some travis raw logs.

// https://s3.amazonaws.com/archive.travis-ci.org/jobs/173808635/log.txt
// https://s3.amazonaws.com/archive.travis-ci.org/jobs/173808636/log.txt
// https://s3.amazonaws.com/archive.travis-ci.org/jobs/173808637/log.txt

[INFO] Reactor Summary:
[INFO] 
[INFO] Zeppelin ........................................... SUCCESS [ 12.258 s]
[INFO] Zeppelin: Interpreter .............................. SUCCESS [ 17.129 s]
[INFO] Zeppelin: Zengine .................................. SUCCESS [ 11.340 s]
[INFO] Zeppelin: Display system apis ...................... SUCCESS [ 19.516 s]
[INFO] Zeppelin: Spark dependencies ....................... SUCCESS [ 44.808 s]
[INFO] Zeppelin: Spark .................................... SUCCESS [ 23.799 s]
[INFO] Zeppelin: Markdown interpreter ..................... SUCCESS [  0.594 s]
[INFO] Zeppelin: Angular interpreter ...................... SUCCESS [  0.270 s]
[INFO] Zeppelin: Shell interpreter ........................ SUCCESS [  0.335 s]
[INFO] Zeppelin: Livy interpreter ......................... SUCCESS [02:34 min]
[INFO] Zeppelin: HBase interpreter ........................ SUCCESS [ 10.105 s]
[INFO] Zeppelin: Apache Pig Interpreter ................... SUCCESS [  5.611 s]
[INFO] Zeppelin: PostgreSQL interpreter ................... SUCCESS [  0.615 s]
[INFO] Zeppelin: JDBC interpreter ......................... SUCCESS [  1.026 s]
[INFO] Zeppelin: File System Interpreters ................. SUCCESS [  1.140 s]
[INFO] Zeppelin: Flink .................................... SUCCESS [  8.944 s]
[INFO] Zeppelin: Apache Ignite interpreter ................ SUCCESS [  1.168 s]
[INFO] Zeppelin: Kylin interpreter ........................ SUCCESS [  0.407 s]
[INFO] Zeppelin: Python interpreter ....................... SUCCESS [  0.397 s]
[INFO] Zeppelin: Lens interpreter ......................... SUCCESS [  4.180 s]
[INFO] Zeppelin: Apache Cassandra interpreter ............. SUCCESS [01:04 min]
[INFO] Zeppelin: Elasticsearch interpreter ................ SUCCESS [  8.569 s]
[INFO] Zeppelin: BigQuery interpreter ..................... SUCCESS [  4.676 s]
[INFO] Zeppelin: Alluxio interpreter ...................... SUCCESS [  3.305 s]
[INFO] Zeppelin: web Application .......................... SUCCESS [01:37 min]
[INFO] Zeppelin: Server ................................... SUCCESS [ 22.082 s]
[INFO] Zeppelin: Packaging distribution ................... SUCCESS [01:04 min]
[INFO] Zeppelin: Scalding interpreter ..................... FAILURE [  1.212 s]
[INFO] Zeppelin: Examples ................................. SKIPPED
[INFO] Zeppelin: Example application - Clock .............. SKIPPED
[INFO] Zeppelin: Example application - Horizontal Bar chart SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------

### What is this PR for? This PR adds support for formatting math formula formatting in %html display system using MathJax library. ### What type of PR is it? Feature ### Todos * [x] - Format math formula with MathJax library ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-777 ### How should this be tested? try run following codes. ``` %md When \$a \\ne 0\$, there are two solutions to \$ax^2 + bx + c = 0\$ and they are $$x = {-b \pm \sqrt{b^2-4ac} \over 2a}.$$ ``` Note MathJax works better with `markdown.parser.type` property set `pegdown`, in markdown interpreter. With default markdown4j parser, some formula is not well displayed. (for example, ax^2) I think this will not be a big problem because of we'll remove markdown4j #1594 ``` %sh echo -e "%html \$\$a = b\$\$" ``` ``` %spark println("%html $$b = c$$") ``` ### Screenshots (if appropriate) ![image](https://cloud.githubusercontent.com/assets/1540981/20040864/3b69c540-a414-11e6-8f8a-fdf7ee1370a6.png) ### Questions: * Does the licenses files need update? yes * Is there breaking changes for older versions? no * Does this needs documentation? yes Author: Lee moon soo <[email protected]> Closes #1606 from Leemoonsoo/ZEPPELIN-777 and squashes the following commits: af8e079 [Lee moon soo] Package MathJax resources 2afedde [Lee moon soo] Fix typo dd02bec [Lee moon soo] Add doc for mathmetical expression 174d7ad [Lee moon soo] Add license bb762c3 [Lee moon soo] Format formula using MathJax

### What is this PR for? This PR adds support for formatting math formula formatting in %html display system using MathJax library. ### What type of PR is it? Feature ### Todos * [x] - Format math formula with MathJax library ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-777 ### How should this be tested? try run following codes. ``` %md When \$a \\ne 0\$, there are two solutions to \$ax^2 + bx + c = 0\$ and they are $$x = {-b \pm \sqrt{b^2-4ac} \over 2a}.$$ ``` Note MathJax works better with `markdown.parser.type` property set `pegdown`, in markdown interpreter. With default markdown4j parser, some formula is not well displayed. (for example, ax^2) I think this will not be a big problem because of we'll remove markdown4j apache#1594 ``` %sh echo -e "%html \$\$a = b\$\$" ``` ``` %spark println("%html $$b = c$$") ``` ### Screenshots (if appropriate) ![image](https://cloud.githubusercontent.com/assets/1540981/20040864/3b69c540-a414-11e6-8f8a-fdf7ee1370a6.png) ### Questions: * Does the licenses files need update? yes * Is there breaking changes for older versions? no * Does this needs documentation? yes Author: Lee moon soo <[email protected]> Closes apache#1606 from Leemoonsoo/ZEPPELIN-777 and squashes the following commits: af8e079 [Lee moon soo] Package MathJax resources 2afedde [Lee moon soo] Fix typo dd02bec [Lee moon soo] Add doc for mathmetical expression 174d7ad [Lee moon soo] Add license bb762c3 [Lee moon soo] Format formula using MathJax

1ambda · 2016-11-10T06:19:00Z

resolve merge conflict with 777.

Now CI is failing on Server :( I will close and re-open this issue to trigger CI

INFO] Zeppelin: web Application .......................... SUCCESS [01:43 min]
[INFO] Zeppelin: Server ................................... FAILURE [02:37 min]

I found the failed test related to markdown

estInterpreterRestart(org.apache.zeppelin.rest.InterpreterRestApiTest)  Time elapsed: 3.016 sec  <<< FAILURE!
org.junit.ComparisonFailure: expected:<<[p>markdown</p>
]> but was:<<[div class="markdown-body">
<p>markdown</p>
</div>]>
    at org.junit.Assert.assertEquals(Assert.java:115)
    at org.junit.Assert.assertEquals(Assert.java:144)
    at org.apache.zeppelin.rest.InterpreterRestApiTest.testInterpreterRestart(InterpreterRestApiTest.java:176)

testRestartInterpreterPerNote(org.apache.zeppelin.rest.InterpreterRestApiTest)  Time elapsed: 0.119 sec  <<< FAILURE!
org.junit.ComparisonFailure: expected:<<[p>markdown</p>
]> but was:<<[div class="markdown-body">
<p>markdown</p>
</div>]>
    at org.junit.Assert.assertEquals(Assert.java:115)
    at org.junit.Assert.assertEquals(Assert.java:144)
    at org.apache.zeppelin.rest.InterpreterRestApiTest.testRestartInterpreterPerNote(InterpreterRestApiTest.java:221)

This reverts commit f704d3e.

This reverts commit a9c619d.

This reverts commit 580233f.

This reverts commit cd45d85.

This reverts commit 226c1e6.

This reverts commit a36c86c.

This reverts commit e9fb0df.

1ambda · 2016-11-11T06:51:46Z

@Leemoonsoo @felixcheung @bzz now CI is green :)

1ambda · 2016-11-15T04:54:42Z

@bzz Please let me know if you need anything to merge this PR :)

bzz · 2016-11-15T08:52:14Z

Looks great to me,
merging to master if there is no further discussion

### What is this PR for? Remove unmanaged, old library markdown4j dep which exists just for Websequence and YUML plugins. (related to apache#1384) By adding Websequence and YUML plugins to pegdown - Removing markdown4j dependency which is unmanaged library currently. - Addtionally, we can remove `markdown.parser.type` options in **markdown interpreter** - Fixing some bugs in Websequence and YUML plugins - Enable others to add more plugins using pegdown sytnax. ### What type of PR is it? Improvement ### Todos Nothing ### What is the Jira issue? [JIRA - ZEPPELIN-1614](https://issues.apache.org/jira/browse/ZEPPELIN-1614) ### How should this be tested? Some functional tests are included. ### Screenshots (if appropriate) ### Questions: * Does the licenses files need update? - YES * Is there breaking changes for older versions? - NO * Does this needs documentation? - YES Author: 1ambda <[email protected]> Closes apache#1594 from 1ambda/feat/remove-markdown4j-dep and squashes the following commits: 5af1607 [1ambda] fix: Failed tests in InterpreterRestApiTest c57fdcb [1ambda] docs: Update markdown.md 5c62236 [1ambda] docs: Update upgrade.md for '%md' a1e779d [1ambda] style: Use zeppelin checkstyle.xml 13e0dc4 [1ambda] Update: interpreter setting and docs de3549d [1ambda] chore: Cleanup duplicated markdown4j license 7c5d41e [1ambda] fix: Parse style param optionally in webseq 8831ca1 [1ambda] fix: Wrap exceptions in catch stmt 9268695 [1ambda] Revert "fix: Cleanup unused Markdown4j Parser" 33fb800 [1ambda] Revert "docs: Remove markdown.parser.type option" fddc459 [1ambda] Revert "chore: Remove markdown4j dep and update license" a59ebbd [1ambda] Revert "fix: Set {} to avoid 503" 4e48933 [1ambda] Revert "fix: Parse style param optionally in webseq" 8cfb2c8 [1ambda] Revert "fix: style and misspell in docs" 73956e0 [1ambda] Revert "fix: Propagate exception in YUML plugin" 1b7787f [1ambda] fix: Propagate exception in YUML plugin c656d08 [1ambda] fix: style and misspell in docs dc4f110 [1ambda] fix: Parse style param optionally in webseq b43e14e [1ambda] fix: Set {} to avoid 503 c48cc53 [1ambda] chore: Remove markdown4j dep and update license 81fdfcc [1ambda] docs: Remove markdown.parser.type option cf19f0b [1ambda] fix: Cleanup unused Markdown4j Parser 98b2809 [1ambda] fix: Add missing docs 3e9716d [1ambda] feat: Yuml markdown plugin 3247c67 [1ambda] feat: Support webseq markdown plugin

bzz reviewed Nov 4, 2016

View reviewed changes

felixcheung reviewed Nov 4, 2016

View reviewed changes

Leemoonsoo mentioned this pull request Nov 6, 2016

[ZEPPELIN-777] Math formula support #1606

Closed

1 task

1ambda force-pushed the feat/remove-markdown4j-dep branch from df24ae9 to 25ae962 Compare November 7, 2016 05:58

1ambda force-pushed the feat/remove-markdown4j-dep branch from 25ae962 to 035c1f8 Compare November 10, 2016 04:35

1ambda closed this Nov 10, 2016

1ambda reopened this Nov 10, 2016

1ambda added 10 commits November 11, 2016 13:28

feat: Support webseq markdown plugin

3247c67

feat: Yuml markdown plugin

3e9716d

fix: Add missing docs

98b2809

fix: Cleanup unused Markdown4j Parser

cf19f0b

docs: Remove markdown.parser.type option

81fdfcc

chore: Remove markdown4j dep and update license

c48cc53

fix: Set {} to avoid 503

b43e14e

fix: Parse style param optionally in webseq

dc4f110

fix: style and misspell in docs

c656d08

fix: Propagate exception in YUML plugin

1b7787f

1ambda added 15 commits November 11, 2016 13:28

Revert "fix: Propagate exception in YUML plugin"

73956e0

This reverts commit f704d3e.

Revert "fix: style and misspell in docs"

8cfb2c8

This reverts commit a9c619d.

Revert "fix: Parse style param optionally in webseq"

4e48933

This reverts commit 580233f.

Revert "fix: Set {} to avoid 503"

a59ebbd

This reverts commit cd45d85.

Revert "chore: Remove markdown4j dep and update license"

fddc459

This reverts commit 226c1e6.

Revert "docs: Remove markdown.parser.type option"

33fb800

This reverts commit a36c86c.

Revert "fix: Cleanup unused Markdown4j Parser"

9268695

This reverts commit e9fb0df.

fix: Wrap exceptions in catch stmt

8831ca1

fix: Parse style param optionally in webseq

7c5d41e

chore: Cleanup duplicated markdown4j license

de3549d

Update: interpreter setting and docs

13e0dc4

style: Use zeppelin checkstyle.xml

a1e779d

docs: Update upgrade.md for '%md'

5c62236

docs: Update markdown.md

c57fdcb

fix: Failed tests in InterpreterRestApiTest

5af1607

1ambda force-pushed the feat/remove-markdown4j-dep branch from 8346417 to 5af1607 Compare November 11, 2016 04:28

1ambda closed this Nov 11, 2016

1ambda reopened this Nov 11, 2016

asfgit closed this in ab2cdfd Nov 16, 2016

1ambda deleted the feat/remove-markdown4j-dep branch November 17, 2016 05:02

[ZEPPELIN-1614] Remove markdown4j dep #1594

[ZEPPELIN-1614] Remove markdown4j dep #1594

Uh oh!

Conversation

1ambda commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzz commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Nov 4, 2016

Uh oh!

1ambda commented Nov 4, 2016

Uh oh!

Leemoonsoo commented Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Leemoonsoo commented Nov 6, 2016

Uh oh!

1ambda commented Nov 7, 2016

Uh oh!

bzz commented Nov 9, 2016

Uh oh!

1ambda commented Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1ambda commented Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1ambda commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1ambda commented Nov 15, 2016

Uh oh!

bzz commented Nov 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

1ambda commented Nov 4, 2016 •

edited

Loading

bzz commented Nov 4, 2016 •

edited

Loading

Leemoonsoo commented Nov 6, 2016 •

edited

Loading

1ambda commented Nov 9, 2016 •

edited

Loading

1ambda commented Nov 10, 2016 •

edited

Loading

1ambda commented Nov 11, 2016 •

edited

Loading