[ZEPPELIN-699] Add new synchronous paragraph run REST API #746

doanduyhai · 2016-02-24T20:33:37Z

What is this PR for?

Right now, when calling the REST API http://<ip>:<port>/api/notebook/job/<note_id>/<paragraph_id> Zeppelin always returns OK as shown by this source code: https://github.com/apache/incubator-zeppelin/blob/master/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java#L477

This ticket will update the behavior so that Zeppelin also return the result of the paragraph execution

What type of PR is it?

[Improvement]

Todos

- Code Review
- Simple Test

Is there a relevant Jira issue?

ZEPPELIN-699

How should this be tested?

git fetch origin pull/746/head:ParagraphExecutionRESTAPI
git checkout ParagraphExecutionRESTAPI
mvn clean package -DskipTests
bin/zeppelin-daemon.sh restart
Create a new note
In the first paragraph, put the following code

%sh

echo "Current time = "`date +"%T"

Retrieve the current note id in the URL
Retrieve the current paragraph id
Use a REST Client like POSTman to create a HTTP POST query http://<ip>:<port>/api/notebook/run/<note_id>/<paragraph_id>
You should receive something similar as follow for answer

{
    "status": "OK",
    "body": {
        "code": "SUCCESS",
        "type": "TEXT",
        "msg": "Current time = 16:14:18\n"
    }
}

Screenshots (if appropriate)

API Documentation update

Existing asynchronous API

New synchronous API

Questions:

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

felixcheung · 2016-02-25T00:45:19Z

zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java

+    if (result.code() == InterpreterResult.Code.SUCCESS) {
+      return new JsonResponse<>(Status.OK, result.message()).build();
+    } else {
+      return new JsonResponse<>(Status.OK).build();


shouldn't it not return OK if its run fails?

Right! Fixing it now

vgkowski · 2016-02-25T09:15:41Z

I would prefer to not return OK in case of failure

doanduyhai · 2016-02-25T13:53:51Z

Fixed

Leemoonsoo · 2016-02-25T14:42:45Z

zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java

+      paragraph.setListener(note.getJobListenerFactory().getParagraphJobListener(note));
+    }
+
+    paragraph.run();


paragraph supposed to not run in this way.
It should always run through Scheduler that Interpreter provides.

@doanduyhai do you plan to address this issue?

Leemoonsoo · 2016-02-25T14:48:15Z

This PR change behavior of REST api job/{notebookId}/{paragraphId} from asynchronous to synchronous.

To me it's better we keep asynchronous behavior for backward compatibility. Some user might depends on asynchronous behavior.

Synchronous behavior can be implemented in a different rest api.

doanduyhai · 2016-02-25T15:10:37Z

@Leemoonsoo so you're suggesting to create another API for pagraph execution ? Fine for me, what's about http://<ip>:<port>/api/notebook/synchronous_job/<note_id>/<paragraph_id> ?

Leemoonsoo · 2016-02-25T15:32:43Z

@doanduyhai Right.

for asynchronous behavior, multiple API is required to control the job, like submitting job (POST), canceling job (DELETE), get status of job (GET) to /api/notebook/job/<note_id>/<paragraph_id>.

but for synchronous behavior, a single API will go through all the lifecycle of the job, from submitting to get result. So how about just say

/api/notebook/run/<note_id>/<paragraph_id> ?

vgkowski · 2016-02-25T15:37:49Z

I would suggest to use the same method as for the spark job server, asynchronous is the default, synchronous is obtain with an additional parameter.

De : Lee moon soo [mailto:[email protected]]
Envoyé : jeudi 25 février 2016 15:48
À : apache/incubator-zeppelin
Cc : GROMAKOWSKI Vincent IST/ISAD
Objet : Re: [incubator-zeppelin] [ZEPPELIN-699] Return Paragraph execution result for the REST API (#746)

This PR change behavior of REST api job/{notebookId}/{paragraphId} from asynchronous to synchronous.

To me it's better we keep asynchronous behavior for backward compatibility. Some user might depends on asynchronous behavior.

Synchronous behavior can be implemented in a different rest api.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/746#issuecomment-188816674.

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.

doanduyhai · 2016-02-25T20:45:27Z

@vgromakowski

Something like http://<ip>:<port>/api/notebook/job/<note_id>/<paragraph_id>?sync=true ?

Leemoonsoo · 2016-02-25T21:08:10Z

I don't have strong opinion here for adding parameter vs new api.
But i want to mention that adding parameter make the api return two different things.

When sync is false, return success or fail for the job submission.
When sync is true, return success or fail for the job execution, with result data.

That makes the api difficult to understand. And later, there could be some additional parameter introduced for only for async api or only for sync api. in that case, combination of different parameters need to be explained for users.

So i'm little bit biased to new api for this reasons. what do you guys think?

doanduyhai · 2016-02-25T21:43:07Z

Humm, your remark about future extension of the existing API with new parameter makes sense. We need to design API carefully to make evolution easier.

This plays in favor for a separate API

By the way, for the existing asynchronous API, I don't think returning OK in all cases is correct. Since the execution of the paragraph is asynchronous and we don't wait for it to return the result to the client, how can we know if the execution has been successful or not ? How can we decide if we should return OK or INTERNAL_SERVER_ERROR ?

felixcheung · 2016-02-25T22:16:50Z

that we would need a job run status API...

felixcheung · 2016-03-13T19:52:10Z

hello - where are we on this?

doanduyhai · 2016-03-13T19:58:39Z

@felixcheung I'm still waiting for someone to reply me on my question:

By the way, for the existing asynchronous API, I don't think returning OK in all cases is correct. Since the execution of the paragraph is asynchronous and we don't wait for it to return the result to the client, how can we know if the execution has been successful or not ? How can we decide if we should return OK or INTERNAL_SERVER_ERROR ?

Or do you think we can just let it as it is and create the new API ?

fireboy1919 · 2016-03-14T19:58:37Z

I have a similar REST endpoint based upon the use of resource pools (i.e., starting from this pull request):
#763 (comment)

I made mine use this form:
http://:/api/notebook/job/<note_id>/<paragraph_id>/message

This is a bit more flexible than pulling straight from the paragraph and does not have synchronous API problem.

Once that new pull request has been approved, I'll format and post my pull request.

felixcheung · 2016-03-14T21:21:03Z

@doanduyhai typically async pattern in REST, it seems, is either:

server callback
list status
So IMO if we submit a request to run paragraph asynchronously, and that request is accepted, then we would get back a 202 (preferred) or 200, and through another request we would check how the run request is going.

felixcheung · 2016-05-13T22:36:39Z

hello - how are we on this?

doanduyhai · 2016-06-02T14:17:18Z

@felixcheung I've updated the PR to create a new API as suggested by @Leemoonsoo : /run/{noteId}/{paragraphId}.

All my apologies for the long delay, I was lately busy with Cassandra

felixcheung · 2016-06-03T05:13:32Z

docs/rest-api/rest-notebook.md

+      <td>URL</td>
+      <td>```http://[zeppelin-server]:[zeppelin-port]/api/notebook/run/[notebookId]/[paragraphId]```</td>
+    </tr>
+    <tr>


could you please fix the extra space for this and next line?

thanks, I'm referring to whitespaces in/around <td> Fail code</td>, <td> 500 </td> or <td> sample JSON input (optional, only needed when if you want to update dynamic form's value) </td> - are they intentional?

doanduyhai · 2016-06-03T08:33:19Z

@felixcheung Done, I've added 2 new screen captures of the API documentation update

doanduyhai · 2016-06-03T13:08:49Z

Hello @bzz, I've rebased from the latest master and my PR always fail with this issue:

12:28:28,504  INFO org.apache.zeppelin.AbstractZeppelinIT:59 - Finished.
Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 83.01 sec - in org.apache.zeppelin.integration.ParagraphActionsIT

Results :

Tests in error: 
  ZeppelinIT.testSparkInterpreterDependencyLoading:243 Â» NoSuchElement Unable to...

You have committed recently a fix for ZeppelinIT, do you know what can trigger this issue ?

I have the exact same error on my other PR for Cassandra interpreter update: #950

doanduyhai · 2016-06-15T10:08:17Z

Rebased from master

doanduyhai · 2016-08-29T14:52:33Z

Ok, there are 3 unrelated errors:

https://s3.amazonaws.com/archive.travis-ci.org/jobs/155941982/log.txt

[32mAngularElem�[0m
�[31m- should provide onclick method *** FAILED ***�[0m
�[31m  The code passed to eventually never returned normally. Attempted 1 times over 208.64569899999998 milliseconds. Last failure message: 0 was not equal to 1. (AbstractAngularElemTest.scala:72)�[0m

The test timeoud because the default implicit for Scala eventually {a should be(1)} is only 150ms. Timeout is a recurrent issue when running in containers in Travis

https://s3.amazonaws.com/archive.travis-ci.org/jobs/155941985/log.txt

Some flaky remote interpreter test. When running locally the test with mvn test -Dtest=org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcessTest it is successful:

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcessTest
16:48:19,231  INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreterManagedProcess:116 - Run interpreter process [../bin/interpreter.sh, -d, nonexists, -p, 64503, -l, fakeRepo]
16:48:20,317  INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcess:127 - shutdown interpreter process
16:48:22,579  INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcess:137 - Exception in RemoteInterpreterProcess while synchronized dereference, can safely ignore exception while client.shutdown() may terminates remote process
16:48:22,580  INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreterManagedProcess:154 - Interpreter process exited 0
16:48:23,103  INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreterServer:89 - Starting remote interpreter server on port 3678
16:48:23,471  INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreterManagedProcess:116 - Run interpreter process [../bin/interpreter.sh, -d, nonexists, -p, 64509, -l, fakeRepo]
16:48:23,979  INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreterManagedProcess:116 - Run interpreter process [../bin/interpreter.sh, -d, nonexists, -p, 64512, -l, fakeRepo]
16:48:24,485  INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcess:127 - shutdown interpreter process
16:48:26,630  INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcess:137 - Exception in RemoteInterpreterProcess while synchronized dereference, can safely ignore exception while client.shutdown() may terminates remote process
16:48:26,632  INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreterManagedProcess:154 - Interpreter process exited 0
Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 8.076 sec - in org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcessTest

Results :

Tests run: 3, Failures: 0, Errors: 0, Skipped: 0

https://s3.amazonaws.com/archive.travis-ci.org/jobs/155942015/log.txt

Just a classical NPM download failure:

[INFO] --- frontend-maven-plugin:0.0.25:npm (npm install) @ zeppelin-web ---
[INFO] Running 'npm install --color=false' in /home/travis/build/apache/zeppelin/zeppelin-web
[ERROR] npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
[ERROR] npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
[ERROR] npm WARN deprecated [email protected]: graceful-fs v3.0.0 and before will fail on node releases >= v7.0. Please update to graceful-fs@^4.0.0 as soon as possible. Use 'npm ls graceful-fs' to find it in the tree.
[ERROR] npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
[ERROR] npm WARN deprecated [email protected]: lodash@<3.0.0 is no longer maintained. Upgrade to lodash@^4.0.0.
[ERROR] npm WARN deprecated [email protected]: graceful-fs v3.0.0 and before will fail on node releases >= v7.0. Please update to graceful-fs@^4.0.0 as soon as possible. Use 'npm ls graceful-fs' to find it in the tree.
[ERROR] npm WARN deprecated [email protected]: ReDoS vulnerability parsing Set-Cookie https://nodesecurity.io/advisories/130
[ERROR] npm WARN deprecated [email protected]: the module is now available as 'css-select'
[ERROR] npm WARN deprecated [email protected]: the module is now available as 'css-what'
[ERROR] npm WARN optional dep failed, continuing [email protected]
[ERROR] npm ERR! Linux 3.13.0-40-generic
[ERROR] npm ERR! argv "/home/travis/build/apache/zeppelin/zeppelin-web/node/node" "/home/travis/build/apache/zeppelin/zeppelin-web/node/node_modules/npm/bin/npm-cli.js" "install" "--color=false"
[ERROR] npm ERR! node v0.12.13
[ERROR] npm ERR! npm  v2.15.0
[ERROR] npm ERR! code ECONNRESET
[ERROR] npm ERR! errno ECONNRESET
[ERROR] npm ERR! syscall read
[ERROR] 
[ERROR] npm ERR! network read ECONNRESET
[ERROR] npm ERR! network This is most likely not a problem with npm itself
[ERROR] npm ERR! network and is related to network connectivity.
[ERROR] npm ERR! network In most cases you are behind a proxy or have bad network settings.
[ERROR] npm ERR! network 
[ERROR] npm ERR! network If you are behind a proxy, please make sure that the
[ERROR] npm ERR! network 'proxy' config is set properly.  See: 'npm help config'
[INFO] 
[INFO] > [email protected] install /home/travis/build/apache/zeppelin/zeppelin-web/node_modules/karma/node_modules/socket.io/node_modules/socket.io-client/node_modules/ws
[INFO] > (node-gyp rebuild 2> builderror.log) || (exit 0)

@felixcheung @jongyoul Can we merge this and move on ? This PR has been there for a while and I have re-based it many times against master

doanduyhai · 2016-08-30T20:12:08Z

Ping @Leemoonsoo , can we merge this long-standing PR ? CI failures are irrelevant as mentioned above

jongyoul · 2016-08-31T10:10:52Z

I've glanced at this PR. I think this PR never break current behaviour and codes looks good. If it doesn't have more issues, it's enough to merge this PR.

felixcheung · 2016-09-01T06:56:53Z

Done!

doanduyhai · 2016-09-01T06:58:49Z

Thank you @felixcheung. Finally merged !

doanduyhai force-pushed the ZEPPELIN-699 branch from 28f2084 to e14f08a Compare February 24, 2016 20:35

felixcheung reviewed Feb 25, 2016
View reviewed changes

doanduyhai force-pushed the ZEPPELIN-699 branch from e14f08a to bdd0788 Compare February 25, 2016 13:09

Leemoonsoo reviewed Feb 25, 2016
View reviewed changes

doanduyhai force-pushed the ZEPPELIN-699 branch from bdd0788 to c8825f7 Compare June 2, 2016 14:15

doanduyhai changed the title ~~[ZEPPELIN-699] Return Paragraph execution result for the REST API~~ [ZEPPELIN-699] Add new synchronous paragraph run REST API Jun 2, 2016

felixcheung reviewed Jun 3, 2016
View reviewed changes

doanduyhai force-pushed the ZEPPELIN-699 branch from a19340c to 46a2f36 Compare June 3, 2016 08:29

doanduyhai closed this Jun 3, 2016

doanduyhai reopened this Jun 3, 2016

doanduyhai closed this Jun 3, 2016

doanduyhai closed this Jun 16, 2016

doanduyhai reopened this Jun 16, 2016

doanduyhai closed this Jun 16, 2016

doanduyhai reopened this Jun 16, 2016

doanduyhai closed this Jun 17, 2016

doanduyhai reopened this Jun 17, 2016

doanduyhai added 2 commits August 26, 2016 09:20

[ZEPPELIN-699] Add new synchronous paragraph run REST API

8367acf

[ZEPPELIN-699] Update Notebook REST API documentation

fb0570c

doanduyhai force-pushed the ZEPPELIN-699 branch from 0106cf5 to fb0570c Compare August 26, 2016 11:01

doanduyhai closed this Aug 26, 2016

doanduyhai reopened this Aug 26, 2016

doanduyhai closed this Aug 28, 2016

doanduyhai reopened this Aug 28, 2016

doanduyhai closed this Aug 28, 2016

doanduyhai reopened this Aug 28, 2016

doanduyhai closed this Aug 28, 2016

doanduyhai reopened this Aug 28, 2016

doanduyhai closed this Aug 28, 2016

doanduyhai reopened this Aug 28, 2016

doanduyhai closed this Aug 29, 2016

doanduyhai reopened this Aug 29, 2016

doanduyhai closed this Aug 29, 2016

doanduyhai reopened this Aug 29, 2016

asfgit closed this in 7e2a1b5 Sep 1, 2016

[ZEPPELIN-699] Add new synchronous paragraph run REST API #746

[ZEPPELIN-699] Add new synchronous paragraph run REST API #746

Uh oh!

Conversation

doanduyhai commented Feb 24, 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

Is there a relevant Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

felixcheung Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

doanduyhai Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

vgkowski commented Feb 25, 2016

Uh oh!

doanduyhai commented Feb 25, 2016

Uh oh!

Leemoonsoo Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

bzz Jun 5, 2016

Choose a reason for hiding this comment

Uh oh!

Leemoonsoo commented Feb 25, 2016

Uh oh!

doanduyhai commented Feb 25, 2016

Uh oh!

Leemoonsoo commented Feb 25, 2016

Uh oh!

vgkowski commented Feb 25, 2016

Uh oh!

doanduyhai commented Feb 25, 2016

Uh oh!

Leemoonsoo commented Feb 25, 2016

Uh oh!

doanduyhai commented Feb 25, 2016

Uh oh!

felixcheung commented Feb 25, 2016

Uh oh!

felixcheung commented Mar 13, 2016

Uh oh!

doanduyhai commented Mar 13, 2016

Uh oh!

fireboy1919 commented Mar 14, 2016

Uh oh!

felixcheung commented Mar 14, 2016

Uh oh!

felixcheung commented May 13, 2016

Uh oh!

doanduyhai commented Jun 2, 2016

Uh oh!

felixcheung Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

doanduyhai Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

felixcheung Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

doanduyhai commented Jun 3, 2016

Uh oh!

doanduyhai commented Jun 3, 2016

Uh oh!

doanduyhai commented Jun 15, 2016

Uh oh!

doanduyhai commented Aug 29, 2016

Uh oh!

doanduyhai commented Aug 30, 2016

Uh oh!

jongyoul commented Aug 31, 2016

Uh oh!

felixcheung commented Sep 1, 2016

doanduyhai commented Feb 24, 2016 •

edited

Loading