ZEPPELIN-335. Pig Interpreter by zjffdu · Pull Request #1476 · apache/zeppelin

zjffdu · 2016-09-30T09:08:59Z

What is this PR for?

Based on #338 , I refactor most of pig interpreter. As I don't think the approach in #338 is the best approach. In #338, we use script bin/pig to launch pig script, it is different to control that job (hard to kill and get progress and stats info). In this PR, I use pig api to launch pig script. Besides that I implement another interpreter type %pig.query to leverage the display system of zeppelin. For the details you can check pig.md

What type of PR is it?

[Feature]

Todos

Syntax Highlight
new interpreter type %pig.udf, so that user can write pig udf in zeppelin directly and don't need to build udf jar manually.

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-335

How should this be tested?

Unit test is added and also manual test is done

Screenshots (if appropriate)

Questions:

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

felixcheung · 2016-10-01T06:55:03Z

+ Launcher launcher = (Launcher) launcherField.get(engine);
+ // It doesn't work for Tez Engine due to PIG-5035
+ launcher.killJob(jobId, new Configuration());
+ } catch (NoSuchFieldException e) {


Add message string or merge like this?
catch (NoSuchFieldException | BackendException | IllegalAccessException e)

zjffdu · 2016-10-07T06:04:56Z

ping @Leemoonsoo @bzz @felixcheung @abajwa-hw @vgmartinez @maparco @smartinsightsfromdata please help review.

felixcheung · 2016-10-07T22:54:37Z

Is it possible have a non-Tez version of Pig and would it work with this interpreter?

zjffdu · 2016-10-07T23:23:57Z

Yes, it supports local, mapreduce and tez engine.

felixcheung · 2016-10-08T00:08:18Z

+ if (!fe.getMessage().contains("Backend error :")) {
+ // If the error message contains "Backend error :", that means the exception is from
+ // backend.
+ return new InterpreterResult(Code.ERROR, ExceptionUtils.getStackTrace(e));


seems like pretty bad user experience to expose call stack in paragraph run result? should we just log in and return e.getMessage() instead?

e.getMessage() doesn't contain useful info for diagnosis. e.g. if you specify a nonexist path, e.getMessage() only get error message Unable to open iterator for alias c which is not useful for users. And in pig grunt (pig interactive tool), user can also see the full stacktrace, so I think it is acceptable to display full stack trace here.

I don't feel strongly about it but there has been numerous request to hide exception stack in interpreter results in other interpreters and we generally do not put exception stack in interpreter results.

felixcheung · 2016-10-08T00:09:03Z

+ if (stats != null) {
+ String errorMsg = PigUtils.extactJobStats(stats);
+ if (errorMsg != null) {
+ LOGGER.debug("Error Message:" + errorMsg);


LOGGER.error("message, e) instead?

felixcheung · 2016-10-08T00:10:04Z

+ jobIds.add(js.getJobId());
+ }
+ return jobIds;
+ } catch (Exception e) {


LOGGER.error?

felixcheung · 2016-10-08T00:11:54Z

+ }
+ },
+ "editor": {
+ "language": "pig"


I didn't find it, does https://highlightjs.org/ supports "pig"?

no, it doesn't support, this is in TODO list.

I'm not sure if you plan to add highlighting to the highlightjs project, since that's what we are using.

felixcheung · 2016-10-08T00:13:28Z

Could you add documentation?

also I think LICENSE file should be updated re: pig and dependencies

zjffdu · 2016-10-08T07:52:36Z

@felixcheung please check pig.md

1. Documentation: added pig.md with interpreter documentation and added pig entry to index.md 2. Added test junit test based on passwd file parsing example here https://pig.apache.org/docs/r0.10.0/start.html#run 3. Removed author tag from comment (this was copied from shell interpreter https://github.com/apache/incubator-zeppelin/blob/master/shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java#L42) 4. Implemented cancel functionality 5. Display output stream in case of error

[ZEPPELIN-335][DOCS] Minor update for pig.md

zjffdu · 2016-10-09T13:23:24Z

Thanks @AhyoungRyu for the help, merged.

felixcheung · 2016-10-09T18:18:01Z

+ </tr>
+ <tr>
+ <td>zeppelin.pig.maxResult</td>
+ <td>20</td>


Is this a bit low at 20, since there isn't a way to retrieve the full result?

Change it to 1000 as the same of spark sql

could you update the doc then on the default value?

felixcheung · 2016-10-09T20:14:34Z

+ fi
+
+ # autodetect TEZ_CONF_DIR
+ TEZ_CONF_DIR = ${TEZ_CONF_DIR:=/etc/tez/conf}


should we check that /etc/tez/conf exists before adding to the classpath?

felixcheung · 2016-10-09T20:29:48Z

+ }
+ try {
+ pigServer = new PigServer(execType);
+ } catch (IOException e) {


could you LOGGER.error even when the exception is rethrown? it's easier to see everything in the log file

felixcheung · 2016-10-09T20:31:28Z

+ }
+ }
+ if (!outputBuilder.toString().isEmpty() || !bytesOutput.toString().isEmpty()) {
+ outputBuilder.append("------------- Pig Output --------------\n");


I'm not sure we have a "header" in other interpreter output - is there a reason this could be useful?

felixcheung · 2016-10-09T20:41:38Z

+ PigScriptListener scriptListener = new PigScriptListener();
+ ScriptState.get().registerListener(scriptListener);
+ listenerMap.put(contextInterpreter.getParagraphId(), scriptListener);
+ pigServer.registerScript(tmpFile.getAbsolutePath());


does registerScript block until execution is complete? otherwise it seems we are getting result output and deleting temp script file early

Yes, it is a block api.

felixcheung · 2016-10-09T20:42:27Z

+
+ @Override
+ public InterpreterResult interpret(String st, InterpreterContext context) {
+ String alias = "paragraph_" + context.getParagraphId().replace("-", "_");


could you add a comment on why this is needed?

felixcheung · 2016-10-09T20:44:17Z

+ // Extract error in the following order
+ // 1. catch FrontendException, FrontendException happens in the query compilation phase.
+ // 2. PigStats, This is execution error
+ // 3. Other errors.


LOGGER.error when e is not FrontendException

felixcheung · 2016-10-09T20:50:24Z

+ PigStats.JobGraph jobPlan = (PigStats.JobGraph) jobPlanField.get(stats);
+
+ if (stats.getReturnCode() == PigRunner.ReturnCode.SUCCESS
+ || stats.getReturnCode() == PigRunner.ReturnCode.PARTIAL_FAILURE) {


we want stats.getReturnCode() == PigRunner.ReturnCode.PARTIAL_FAILURE here?

I think so, these code are copied from pig as it is private in pig, PIG-5037 is for exposing such api.

just that stats.getReturnCode() == PigRunner.ReturnCode.PARTIAL_FAILURE is in both the "SUCCESS" and "FAILURE" cases. So if I understand correctly, if return code is PARTIAL_FAILURE, it will say "Job Stats" and "Failed Jobs" at the same time

That's correct, if it is PARTIAL_FAILURE, it would display the job stats of both succeeded jobs and failed jobs. e.g. one pig script needs to run 2 mapreduce jobs, and one job is successful and another is failed.

felixcheung · 2016-10-09T20:55:06Z

how about LICENSE file?

zjffdu · 2016-10-10T00:27:56Z

@felixcheung Thanks for the review, license is added and comments are addressed.

felixcheung · 2016-10-10T05:28:22Z

Thanks, comment replied, in particular, this

felixcheung · 2016-10-10T17:29:59Z

Do you intent to include the TODO in this PR, or as a follow up?
Otherwise I think this is good to go - you can remove the WIP in the title and I'll merge if there is no more comment.

SELENIUM tests seem to be failing consistently, could you check it out?

AhyoungRyu · 2016-10-11T01:45:30Z

 elasticsearch org.apache.zeppelin:zeppelin-elasticsearch:0.6.1 Elasticsearch interpreter
 file org.apache.zeppelin:zeppelin-file:0.6.1 HDFS file interpreter
 flink org.apache.zeppelin:zeppelin-flink_2.11:0.6.1 Flink interpreter built with Scala 2.11
+pig org.apache.zeppelin:zeppelin-pig:0.6.1 Pig interpreter


@zjffdu Could you put this in alphabetic order? :)

zjffdu · 2016-10-11T02:46:51Z

Thanks @felixcheung , Do you know how to run SELENIUM test ? I follow the instruction on READM.MD, but fails

Zeppelin comes with a set of end-to-end acceptance tests driving headless selenium browser
```sh
# assumes zeppelin-server running on localhost:8080 (use -Durl=.. to override)
mvn verify
# or take care of starting/stoping zeppelin-server from packaged zeppelin-distribuion/target
mvn verify -P using-packaged-distr

felixcheung · 2016-10-11T06:45:00Z

Try the commandline from Travis?
https://github.com/apache/zeppelin/blob/master/.travis.yml#L75

zjffdu · 2016-10-11T08:29:57Z

I did it, but fails with the following error:

*** RUN ABORTED ***
java.lang.RuntimeException: Unable to load a Suite class that was discovered in the runpath: org.apache.zeppelin.AbstractFunctionalSuite
at org.scalatest.tools.DiscoverySuite$.getSuiteInstance(DiscoverySuite.scala:84)
at org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:38)
at org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:37)
at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
at scala.collection.Iterator$class.foreach(Iterator.scala:727)
at scala.collection.AbstractIterator.foreach(Iterator.scala:1157)
at scala.collection.IterableLike$class.foreach(IterableLike.scala:72)
at scala.collection.AbstractIterable.foreach(Iterable.scala:54)
at scala.collection.TraversableLike$class.map(TraversableLike.scala:244)
...
Cause: java.lang.RuntimeException: Could not initialize any driver
at org.apache.zeppelin.AbstractFunctionalSuite.getDriver(AbstractFunctionalSuite.scala:64)
at org.apache.zeppelin.AbstractFunctionalSuite.<init>(AbstractFunctionalSuite.scala:39)
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.lang.reflect.Constructor.newInstance(Constructor.java:422)
at java.lang.Class.newInstance(Class.java:442)
at org.scalatest.tools.DiscoverySuite$.getSuiteInstance(DiscoverySuite.scala:69)
at org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:38)
at org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:37)

@Leemoonsoo @bzz Do you know how to run SELENIUM test locally ? Thanks

zjffdu · 2016-10-12T12:14:32Z

@felixcheung Test is passed, I suspect the selenium test is flaky so failed last time.

abajwa-hw · 2016-10-12T14:55:37Z

@zjffdu thanks for working on this interpreter! Looks great

felixcheung · 2016-10-12T18:25:33Z

I think @AhyoungRyu has a comment?

zjffdu · 2016-10-13T02:07:38Z

@felixcheung @AhyoungRyu 's comment is addressed.

felixcheung · 2016-10-13T07:02:31Z

merging if no more comment

### What is this PR for? Based on apache#338 , I refactor most of pig interpreter. As I don't think the approach in apache#338 is the best approach. In apache#338, we use script `bin/pig` to launch pig script, it is different to control that job (hard to kill and get progress and stats info). In this PR, I use pig api to launch pig script. Besides that I implement another interpreter type `%pig.query` to leverage the display system of zeppelin. For the details you can check `pig.md` ### What type of PR is it? [Feature] ### Todos * Syntax Highlight * new interpreter type `%pig.udf`, so that user can write pig udf in zeppelin directly and don't need to build udf jar manually. ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-335 ### How should this be tested? Unit test is added and also manual test is done ### Screenshots (if appropriate) ![image](https://cloud.githubusercontent.com/assets/164491/18986649/54217b4c-8730-11e6-9e33-25f98a98a9b6.png) ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: Jeff Zhang <zjffdu@apache.org> Author: Ali Bajwa <abajwa@hortonworks.com> Author: AhyoungRyu <ahyoungryu@apache.org> Author: Jeff Zhang <zjffdu@gmail.com> Closesapache#1476 from zjffdu/ZEPPELIN-335 and squashes the following commits: 73a07f0 [Jeff Zhang] minor update a1b742b [Jeff Zhang] minor update on doc e858301 [Jeff Zhang] address comments c85a090 [Jeff Zhang] add license 58b4b2f [Jeff Zhang] minor update of docs 1ae7db2 [Jeff Zhang] Merge pull request apache#2 from AhyoungRyu/ZEPPELIN-335/docs fe014a7 [AhyoungRyu] Fix docs title in front matter df7a6db [AhyoungRyu] Add pig.md to dropdown menu 5e2e222 [AhyoungRyu] Minor update for pig.md 39f161a [Jeff Zhang] address comments 05a3b9b [Jeff Zhang] add pig.md a09a7f7 [Jeff Zhang] refactor pig Interpreter c28beb5 [Ali Bajwa] Updated based on comments: 1. Documentation: added pig.md with interpreter documentation and added pig entry to index.md 2. Added test junit test based on passwd file parsing example here https://pig.apache.org/docs/r0.10.0/start.html#run 3. Removed author tag from comment (this was copied from shell interpreter https://github.com/apache/incubator-zeppelin/blob/master/shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java#L42) 4. Implemented cancel functionality 5. Display output stream in case of error 2586336 [Ali Bajwa] exposed timeout and pig executable via interpreter and added comments 7abad20 [Ali Bajwa] initial commit of pig interpreter

Closes#338 (fixed by #1476) Closes#1522 (fixed by #1559) Closes#1527 (fixed by #1511)

zjffdu force-pushed the ZEPPELIN-335 branch 2 times, most recently from f74280e to 908574fCompare September 30, 2016 09:11

zjffdu mentioned this pull request Sep 30, 2016
ZEPPELIN-335: Apache Pig interpreter #338
Closed

zjffdu force-pushed the ZEPPELIN-335 branch from 908574f to 5851f2bCompare September 30, 2016 09:43

felixcheung reviewed Oct 1, 2016
View reviewed changes

zjffdu force-pushed the ZEPPELIN-335 branch from 9b9596a to b98eff1Compare October 7, 2016 02:50

zjffdu closed this Oct 7, 2016

zjffdu reopened this Oct 7, 2016

felixcheung reviewed Oct 8, 2016
View reviewed changes

zjffdu force-pushed the ZEPPELIN-335 branch from 773df9d to 1ba74e4Compare October 8, 2016 03:48

abajwa-hwand others added 5 commits October 8, 2016 16:13

initial commit of pig interpreter
7abad20

exposed timeout and pig executable via interpreter and added comments
2586336

refactor pig Interpreter
a09a7f7

add pig.md
05a3b9b

zjffdu force-pushed the ZEPPELIN-335 branch 2 times, most recently from 9185339 to 8fc5d6fCompare October 8, 2016 08:34

address comments
39f161a

zjffdu force-pushed the ZEPPELIN-335 branch from 8fc5d6f to 39f161aCompare October 8, 2016 08:40

AhyoungRyu added 3 commits October 9, 2016 21:35

Minor update for pig.md
5e2e222

Add pig.md to dropdown menu
df7a6db

Fix docs title in front matter
fe014a7

Merge pull request#2from AhyoungRyu/ZEPPELIN-335/docs
1ae7db2
[ZEPPELIN-335][DOCS] Minor update for pig.md

minor update of docs
58b4b2f

felixcheung requested changes Oct 9, 2016
View reviewed changes

zjffdu added 2 commits October 10, 2016 07:45

add license
c85a090

address comments
e858301

minor update on doc
a1b742b

zjffdu changed the title ~~[WIP] ZEPPELIN-335. Pig Interpreter~~ZEPPELIN-335. Pig InterpreterOct 11, 2016

AhyoungRyu reviewed Oct 11, 2016
View reviewed changes

minor update
73a07f0

zjffdu closed this Oct 12, 2016

zjffdu reopened this Oct 12, 2016

zjffdu closed this Oct 12, 2016

zjffdu reopened this Oct 12, 2016

asfgit closed this in 465c51aOct 15, 2016

asfgit pushed a commit that referenced this pull request Nov 2, 2016
Close PRs on Github without commiting
794c66b
Closes#338 (fixed by #1476) Closes#1522 (fixed by #1559) Closes#1527 (fixed by #1511)

Uh oh!

Conversation

zjffdu commented Sep 30, 2016

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!

zjffdu commented Oct 7, 2016

Uh oh!

felixcheung commented Oct 7, 2016

Uh oh!

zjffdu commented Oct 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Oct 8, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zjffdu commented Oct 8, 2016

Uh oh!

zjffdu commented Oct 9, 2016

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

felixcheungOct 9, 2016•
edited
Loading

felixcheung commented Oct 8, 2016•
edited
Loading

felixcheungOct 9, 2016•
edited
Loading

felixcheungOct 10, 2016•
edited
Loading

zjffduOct 9, 2016•
edited
Loading

felixcheung commented Oct 10, 2016•
edited
Loading