Rest Apis to export/import with common code base #614

ramaswamydevarajan · 2016-01-09T00:27:11Z

Rest apis to export notebook as JSON
Rest api to import notebook from JSON
https://issues.apache.org/jira/browse/ZEPPELIN-590

ramaswamydevarajan · 2016-01-09T00:35:05Z

attaching screenshot for export JUnits

ramaswamydevarajan · 2016-01-09T00:35:38Z

attaching screenshots for import Junit

Leemoonsoo · 2016-01-09T00:40:36Z

Thanks @swakrish for the contribution!
Do you mind update docs/rest-api/rest-notebook.md, too? Changing this file updates zeppelin website.

ramaswamydevarajan · 2016-01-09T01:00:40Z

I added them

bzz · 2016-01-09T03:13:30Z

Thank you for improvement!

Reconciling the exisiting GUI import/export features to use this API may be a good idea too, otherwise we'll have different code doing the same job. What do you think?

Buy the way, could you also please check that your code formated accodring to the project styleguide?

ramaswamydevarajan · 2016-01-09T03:56:48Z

@bzz thanks for your comments. I did look at the UI functions, it doesnt have an import I guess and export is more or less similar. Let me know your thoughts

I also formatted it for google style.

felixcheung · 2016-01-11T01:34:21Z

I think maybe import should be a POST? In REST, PUT should be modifying an existing object?
http://restcookbook.com/HTTP%20Methods/put-vs-post/

ramaswamydevarajan · 2016-01-11T03:12:25Z

@felixcheung Yes makes sense, I changed it to http POST.

Please review and let me know your comments

felixcheung · 2016-01-11T03:34:20Z

Cool thanks.

This is the PR for notebook import #374
Agree with @bzz I think it makes sense if that is using this new REST API too?

ramaswamydevarajan · 2016-01-11T03:55:54Z

@felixcheung , @bzz Does it make sense to have a separate PR for making UI import use REST API?

corneadoug · 2016-01-11T03:57:14Z

Right, it would make sense that both WebSocketAPI and REST API use the same function for Import.

corneadoug · 2016-01-11T03:58:42Z

Just to clarify, the UI should not use the REST API, the WebsocketAPI and the REST API should rely on the same code to do the import on the backend

ramaswamydevarajan · 2016-01-11T04:25:41Z

@corneadoug thanks for the comments, Just to clarify, I would be touchig the notebook.java to create a import/export method which can then be leveraged by Notebookserver.java for UI right?

Please correct me if im wrong

corneadoug · 2016-01-11T04:28:49Z

@swakrish Right exactly, that method would be used in both Websocket and REST API case :)

ramaswamydevarajan · 2016-01-11T05:36:07Z

@corneadoug I moved the code to common notebook.java. Please review and let me know. Thanks

corneadoug · 2016-01-11T05:48:29Z

Shouldn't the REST API send back an error response when something goes bad?

ramaswamydevarajan · 2016-01-11T06:07:02Z

@corneadoug
it does, it throws IOException which results in Http 500 for import failed. This is similar to all other rest api method calls

I added a id not found for export too now, so it should return HTTP 500 too

ramaswamydevarajan · 2016-01-11T22:19:44Z

@corneadoug i donno why travis is failing. it looks like its failing on lots of PRs. Let me know if something else is missing for my commit too

bzz · 2016-01-12T01:04:21Z

There are couple of interesting things in logs:

At the very end:

9479K downloaded
[INFO] Expanding: /home/travis/build/apache/incubator-zeppelin/spark-dependencies/target/spark-dist/spark-1.1.1.tgz into /home/travis/build/apache/incubator-zeppelin/spark-dependencies/target/spark-dist
The log length has exceeded the limit of 4 Megabytes (this usually means that test suite is raising the same exception over and over).
The build has been terminated.

Right before that, there are tests with strange exceptions

9:39:00,009 ERROR org.quartz.core.JobRunShell:211 - Job note.2BB4UKWE4 threw an unhandled Exception: 
java.lang.NullPointerException
at org.apache.zeppelin.notebook.Notebook$CronJob.execute(Notebook.java:484)
at org.quartz.core.JobRunShell.run(JobRunShell.java:202)
at org.quartz.simpl.SimpleThreadPool$WorkerThread.run(SimpleThreadPool.java:573)
09:39:00,010 ERROR org.quartz.core.ErrorLogger:2425 - Job (note.2BB4UKWE4 threw an exception.
org.quartz.SchedulerException: Job threw an unhandled exception. [See nested exception: java.lang.NullPointerException]
at org.quartz.core.JobRunShell.run(JobRunShell.java:213)
at org.quartz.simpl.SimpleThreadPool$WorkerThread.run(SimpleThreadPool.java:573)
Caused by: java.lang.NullPointerException
at org.apache.zeppelin.notebook.Notebook$CronJob.execute(Notebook.java:484)
at org.quartz.core.JobRunShell.run(JobRunShell.java:202)
... 1 more
09:39:00,001 ERROR org.quartz.core.JobRunShell:211 - Job note.2BB4UKWE4 threw an unhandled Exception: 
java.lang.NullPointerException
at org.apache.zeppelin.notebook.Notebook$CronJob.execute(Notebook.java:484)
at org.quartz.core.JobRunShell.run(JobRunShell.java:202)
at org.quartz.simpl.SimpleThreadPool$WorkerThread.run(SimpleThreadPool.java:573)
09:39:00,010 ERROR org.quartz.core.ErrorLogger:2425 - Job (note.2BB4UKWE4 threw an exception.
org.quartz.SchedulerException: Job threw an unhandled exception. [See nested exception: java.lang.NullPointerException]
at org.quartz.core.JobRunShell.run(JobRunShell.java:213)
at org.quartz.simpl.SimpleThreadPool$WorkerThread.run(SimpleThreadPool.java:573)
Caused by: java.lang.NullPointerException
at org.apache.zeppelin.notebook.Notebook$CronJob.execute(Notebook.java:484)
at org.quartz.core.JobRunShell.run(JobRunShell.java:202)
... 1 more

another one, strange

09:39:05,622 ERROR org.apache.zeppelin.rest.NotebookRestApi:341 - Exception in NotebookRestApi while moveParagraph 
java.lang.IndexOutOfBoundsException: paragraph size is 2 , index is 10
at org.apache.zeppelin.notebook.Note.moveParagraph(Note.java:265)
at org.apache.zeppelin.rest.NotebookRestApi.moveParagraph(NotebookRestApi.java:335)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at org.apache.cxf.service.invoker.AbstractInvoker.performInvocation(AbstractInvoker.java:180)

one more suspicious thing

09:39:57,491  INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreterServer:551 - Exception in RemoteInterpreterServer while angularObjectAdd, nolock
com.google.gson.JsonSyntaxException: java.lang.IllegalStateException: Expected BEGIN_OBJECT but was STRING at line 1 column 5
at com.google.gson.Gson.fromJson(Gson.java:802)
at com.google.gson.Gson.fromJson(Gson.java:757)
at com.google.gson.Gson.fromJson(Gson.java:706)
at org.apache.zeppelin.interpreter.remote.RemoteInterpreterServer.angularObjectAdd(RemoteInterpreterServer.java:546)
at org.apache.zeppelin.interpreter.thrift.RemoteInterpreterService$Processor$angularObjectAdd.getResult(RemoteInterpreterService.java:1231)
at org.apache.zeppelin.interpreter.thrift.RemoteInterpreterService$Processor$angularObjectAdd.getResult(RemoteInterpreterService.java:1216)
at org.apache.thrift.ProcessFunction.process(ProcessFunction.java:39)
at org.apache.thrift.TBaseProcessor.process(TBaseProcessor.java:39)
at org.apache.thrift.server.TThreadPoolServer$WorkerProcess.run(TThreadPoolServer.java:285)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.IllegalStateException: Expected BEGIN_OBJECT but was STRING at line 1 column 5
at com.google.gson.stream.JsonReader.expect(JsonReader.java:339)
at com.google.gson.stream.JsonReader.beginObject(JsonReader.java:322)
at com.google.gson.internal.bind.MapTypeAdapterFactory$Adapter.read(MapTypeAdapterFactory.java:184)
at com.google.gson.internal.bind.MapTypeAdapterFactory$Adapter.read(MapTypeAdapterFactory.java:146)
at com.google.gson.Gson.fromJson(Gson.java:791)
... 11 more

Each of them looks very strange to me, never seen any of them before and each worth further investigation.

ramaswamydevarajan · 2016-01-12T03:10:11Z

@bzz
are any related to this PR ? I ran my unit tests standalone and seems to be working fine
point 2, seems to have happened in passed travis builds

bzz · 2016-01-12T04:22:46Z

@swakrish I assume that you, as an author, should know better.

Usually the mindset we try to share here is - CI must be green for anything to be merged. If it's not - then dedicated effort worth putting is - try to fix it i.e by collaborating with more experienced members, asking for help, etc.

If you have some spare cycles you could spend them creating a separate issues, investigating these failures and pinging the people who touched particular code before - that would be very appreciated and of the great help to the project.

Otherwise best bet is to wait until somebody takes care of this one day, so you can rebase on top of that work, but this might take some time.

ramaswamydevarajan · 2016-01-12T17:07:07Z

@bzz thanks for your comments, i just wanted to confirm since when i run test suite it doesnt break any of my changes :)

I will take a look at those failures you mentioned to see why those are happening. Again those doesnt seem to happen in local builds

Merge

bzz · 2016-01-15T09:33:13Z

Sorry for a delay. Will review this today.

ramaswamydevarajan · 2016-01-16T06:13:58Z

thanks @bzz

bzz · 2016-01-16T10:33:17Z

@swakrish sorry, may be I did not make it clear last time, overall your code looks good, except for having 2 code paths for same function (for importing\exporting notebooks) in different APIs (WS\REST) in the same project does not look as a good idea to me.

@bzz Please take a look at notebook.java, this is accessible both from REST and Websocket

I did, and both of them are called exactly once, in the NotebookRestApi.java

This brings us back to the question asked before:

One more thing - @swakrish @corneadoug could you guys please point me to the place where REST API and WS API for importing\exporting notebooks re-use the same code path?

As far as I can tell such place does not exist, right now. This makes concerns, expressed by me and @corneadoug in this PR not addressed. Please let me know if I'm missing something here.

If you have a plan on how to converge these API's code paths (yours and one from the Websocket) - why not to do it in this PR? I think that way it would be even greater contribution, reducing number of possible bugs, etc. What do you think?

ramaswamydevarajan · 2016-01-16T17:31:37Z

@bzz Yes i was thinking of opening a different PR for that since this address just REST calls. I have a tested code base for modifying web socket too
Let me know if creating a separate PR doesnt make sense ? JIRA:https://issues.apache.org/jira/browse/ZEPPELIN-603
snippet:

protected Note importNote(NotebookSocket conn, Notebook notebook, Message fromMessage)
  throws IOException {
Note note = null;
if (fromMessage != null) {
  String noteName = (String) ((Map) fromMessage.get("notebook")).get("name");
  String noteJson = gson.toJson(fromMessage.get("notebook"));
  note = notebook.importNote(noteJson, noteName);
  note.persist();
  broadcastNote(note);
  broadcastNoteList();
}
return note;

}

Merging

ramaswamydevarajan · 2016-01-16T19:49:07Z

@bzz I merged the websocket import to use the common code.So now both REST/UI import uses same code.
Export is just getting the json notation of the note, so I feeel we dont need to change the code base to go through same path. Let me know if you still feel we need to create a websocket call

ramaswamydevarajan · 2016-01-18T19:11:01Z

@bzz can you review this?

bzz · 2016-01-19T03:11:54Z

@swakrish import code looks great.

Just to double-check, I think it's good to ask original author of ImportNote from #374 to review this.
@prabhjyotsingh if you have a few spare cycles - could you take a look please?

prabhjyotsingh · 2016-01-19T05:29:39Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java

sourceJSON -> sourceJson

prabhjyotsingh · 2016-01-19T05:53:09Z

Tested this PR, both newer and existing functionality works fine.

ramaswamydevarajan · 2016-01-19T05:55:33Z

@prabhjyotsingh , thanks for checking this so quickly and your comments.
I modified the comment

Merging

ramaswamydevarajan · 2016-01-20T16:02:35Z

@prabhjyotsingh @bzz , please review

ramaswamydevarajan · 2016-01-21T05:56:26Z

@prabhjyotsingh @bzz is there any other change needed on this PR?

bzz · 2016-01-21T15:32:16Z

@prabhjyotsingh thanks for prompt reply!

Looks great to me, will merge if there is no more discussion.
@swakrish thank you for contributing

felixcheung · 2016-01-21T20:32:51Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java

suggest noteId + " not found" for extra space

Merging

ramaswamydevarajan · 2016-01-21T23:05:07Z

@felixcheung pleasereview, i think travis is stuck :)

felixcheung · 2016-01-21T23:59:19Z

looks good

add keyboard shortcut for toggling run paragraph

ramaswamydevarajan · 2016-01-22T01:40:55Z

@felixcheung thanks for reviewing, looks like travis finally passed

bzz · 2016-01-22T09:15:05Z

@swakrish could you assign a JIRA issues to yourself please? Or let me know your JIRA id and I can do that for you.

Rest Apis to export/import

3645354

ramdevarajan added 2 commits January 8, 2016 16:57

add documentation

0e94dce

fixed alignments

9b64a66

Formatting for google style

ecb8f1e

Changed http put to Http post for REST import

6c19668

Moved export/import methods to Notebook.java

7351f31

added note not found check for export

db8b016

Merge pull request #1 from apache/master

b080d7d

Merge

ramaswamydevarajan added 2 commits January 16, 2016 10:57

Changed websocket import to use common code

e282958

Merge pull request #4 from apache/master

4cb69be

Merging

prabhjyotsingh reviewed Jan 19, 2016
View reviewed changes

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java Outdated

Copy link

Contributor

prabhjyotsingh Jan 19, 2016

Choose a reason for hiding this comment

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

sourceJSON -> sourceJson

changed soureJSON to sourceJson

270e17b

Merge branch 'master' of [email protected]:swakrish/incubator-zeppelin.git

f8a992c

Merge pull request #5 from apache/master

f8bf1f3

Merging

ramaswamydevarajan changed the title ~~Rest Apis to export/import~~ Rest Apis to export/import with common code base Jan 19, 2016

felixcheung reviewed Jan 21, 2016
View reviewed changes

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java Outdated

Copy link

Member

felixcheung Jan 21, 2016

Choose a reason for hiding this comment

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

suggest noteId + " not found" for extra space

ramaswamydevarajan added 3 commits January 21, 2016 13:22

logging the error

869b48f

Merge branch 'master' of [email protected]:swakrish/incubator-zeppelin.git

f5b0805

Merge pull request #6 from apache/master

042d9af

Merging

Merge pull request #7 from apache/master

83e7a6d

add keyboard shortcut for toggling run paragraph

asfgit closed this in 2714d28 Jan 22, 2016

Rest Apis to export/import with common code base #614

Rest Apis to export/import with common code base #614

Uh oh!

Conversation

ramaswamydevarajan commented Jan 9, 2016

Uh oh!

ramaswamydevarajan commented Jan 9, 2016

Uh oh!

ramaswamydevarajan commented Jan 9, 2016

Uh oh!

Leemoonsoo commented Jan 9, 2016

Uh oh!

ramaswamydevarajan commented Jan 9, 2016

Uh oh!

bzz commented Jan 9, 2016

Uh oh!

ramaswamydevarajan commented Jan 9, 2016

Uh oh!

felixcheung commented Jan 11, 2016

Uh oh!

ramaswamydevarajan commented Jan 11, 2016

Uh oh!

felixcheung commented Jan 11, 2016

Uh oh!

ramaswamydevarajan commented Jan 11, 2016

Uh oh!

corneadoug commented Jan 11, 2016

Uh oh!

corneadoug commented Jan 11, 2016

Uh oh!

ramaswamydevarajan commented Jan 11, 2016

Uh oh!

corneadoug commented Jan 11, 2016

Uh oh!

ramaswamydevarajan commented Jan 11, 2016

Uh oh!

corneadoug commented Jan 11, 2016

Uh oh!

ramaswamydevarajan commented Jan 11, 2016

Uh oh!

ramaswamydevarajan commented Jan 11, 2016

Uh oh!

bzz commented Jan 12, 2016

Uh oh!

ramaswamydevarajan commented Jan 12, 2016

Uh oh!

bzz commented Jan 12, 2016

Uh oh!

ramaswamydevarajan commented Jan 12, 2016

Uh oh!

bzz commented Jan 15, 2016

Uh oh!

ramaswamydevarajan commented Jan 16, 2016

Uh oh!

bzz commented Jan 16, 2016

Uh oh!

ramaswamydevarajan commented Jan 16, 2016

Uh oh!

ramaswamydevarajan commented Jan 16, 2016

Uh oh!

ramaswamydevarajan commented Jan 18, 2016

Uh oh!

bzz commented Jan 19, 2016

Uh oh!

prabhjyotsingh Jan 19, 2016

Choose a reason for hiding this comment

Uh oh!

prabhjyotsingh commented Jan 19, 2016

Uh oh!

ramaswamydevarajan commented Jan 19, 2016

Uh oh!

ramaswamydevarajan commented Jan 20, 2016

Uh oh!

ramaswamydevarajan commented Jan 21, 2016

Uh oh!

bzz commented Jan 21, 2016

Uh oh!

felixcheung Jan 21, 2016

Choose a reason for hiding this comment

Uh oh!