Added ipfsnotebookrepo [ Zeppelin-683 ] by onkarshedge · Pull Request #989 · apache/zeppelin

onkarshedge · 2016-06-10T10:42:30Z

What is this PR for?

IPFSNoteBookRepo IPFS storage and sharing zeppelin notes.

What type of PR is it?

Feature

Todos

checkpoint revisions, get particular revision
List of revision history
remove note and also a particular revision
support to import the Note when hash is given as input
Documentation

What is the Jira issue?

Jira issue

How should this be tested?

Installing ipfs

Download and install ipfs for your OS.
Enter the following commands
- ipfs init
- ipfs daemon

ipfs daemon will start the ipfs. In the output you will see the following.

API server listening on /ip4/127.0.0.1/tcp/5001

Gateway (readonly) server listening on /ip4/127.0.0.1/tcp/8080

You can edit the following ports with ipfs config edit.
As zeppelin also uses 8080 by default you might want to chage the gateway server or run zeppelin on another port.

Screenshots (if appropriate)

Successful import
Failure import

Questions:

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

onkarshedge · 2016-06-10T10:43:07Z

@bzz Please review

jongyoul · 2016-06-10T11:13:48Z

- <source>1.7</source>
- <target>1.7</target>
+ <source>1.8</source>
+ <target>1.8</target>


Zeppelin uses jdk 1.7 by default. could you please revert this value?

@jongyoul the ipfs jar I use requires jdk 1.8 I also mentioned this in the google document linked above.

@jongyoul Yes I will revert that value to 1.7 with the next commit. But for this ipfs code to execute the "mvn clean package" must be with jdk 1.8.

I think the best bet right now is try to re-build ipfs jar with target 1.7 locally

bzz · 2016-06-15T07:34:55Z

Great job @onkarshedge !

I understand that it's WIP, just wanted to remind that eventually your NotebookRepo implementation should user NotebookRepoVersioned, see Git one as an example.

Could you also point to some document with high-level description of the approach how notebooks are stored in IPFS, what IPFS features are used and how to view them without Zeppelin, to make sure they are saved for people who are not very familiar with IPFS?

Also, we need to know what is overall current implementation status, like what are TODOs, what is done, what is left, etc? This could be added i.e to the GDoc that you have linked

onkarshedge · 2016-06-15T10:08:14Z

@bzz I had a doubt about versioning approach.
Currently I am saving(ipfs add 'note') the note in IPFS for every call to save function. This gives me a new multihash on saving. So for versioning I have to save the multihash with time. The GitNotebookRepo has a checkpoint which corresponds to git commit. Here in ipfs each call to ipfs add is like git commit. So the List from history function will be large because save() method is called multiple times.
So the List should contain only those few multihashes which user wants to choose.

onkarshedge · 2016-06-15T10:20:14Z

Should i save to Filesystem like VFSNotebookRepo and only when user chooses to version it add it to ipfs that way the ipfs will contain only the ipfs object corresponding to versioned notes and not all through the save method ?
@bzz should IPFSNotebookRepo extends VFSNotebookRepo ?

bzz · 2016-06-20T01:21:45Z

Hi @onkarshedge sorry for late responce, it's your choice how to support versioning of course, but as mid-term evaluation starts soon, I think it's crucial to get answers on 2 previous questions now.

Could you also point to some document with high-level description of the approach how notebooks are stored in IPFS, what IPFS features are used and how to view them without Zeppelin, to make sure they are saved for people who are not very familiar with IPFS?

and

Also, we need to know what is overall current implementation status, like what are TODOs, what is done, what is left, etc? This could be added i.e to the GDoc that you have linked

As soon as that is done it will be easier to help you with further improvements.

onkarshedge · 2016-06-20T06:44:22Z

@bzz Here is the link to document for first question.
High level description
TODO's is the frontend api : To enter the ipfs hash to download the note.

bzz · 2016-06-21T04:29:11Z

Thank you very much @onkarshedge 👍 ! I will go though it and let you know

bzz · 2016-07-07T03:53:50Z

@onkarshedge sorry, people were busy releasing 0.6.0 recently

Could you please:

rebase this code on latest master branch
update PR description to contain full mardown-formatted TODO list: checked ellements for things that are laredy implemented and un-checked for things that are missing (i.e do you plan to add documentation section to storage docs, and so on, for everything that is left.)
post here your current status - like what is in-prgress right now or is there anything that blocks your progress, etc

On your UI proposal

thank you for sharing a great mock! it's very helpfull for a discussion
menu structure in Zeppelin have changed recently
I do not think it is reasonable to add a separate menu for IPFS.
We should provide configuration the same way we do for other notebookRepos. Other than that - we should support User Experience with versions the same way as GitNotebookRepo does - so user can commit changes with message and list them.

Right now listing notebook history in UI is not implemented, but it will be very soon, AFAIK @khalidhuseynov is working on it under ZEPPELIN-1044 so it should "just work" with IpfsNotebookRepo

bzz · 2016-07-12T07:55:24Z

ping, @onkarshedge are there any updates for the past week?

onkarshedge · 2016-07-12T10:20:44Z

@bzz right now I am working on Bittorrent support. I am using the jlibtorrent which uses libtorrent. The past week I spent time on using this library and some examples.I will post a PR of bittorrent in 2-3 days. Sorry for late response.
I didn't work on ipfs in past week.

bzz · 2016-07-14T06:29:24Z

Got it, thanks! Sounds great, please do not forget to submit another WIP pr with your progress on Bittorents support.

Could you still follow #989 (comment) and update this PR as soon as you get back to IPFS one?

Thanks again!

bzz · 2016-08-03T09:09:47Z

ping @onkarshedge

Could you please follow #989 (comment) and update this PR asap?

onkarshedge · 2016-08-04T07:48:16Z

@bzz To import the note by hash url . I will have to use javascript-api in this notenameImport.controller.js file.
How do you suggest to use this?.

bzz · 2016-08-09T06:15:56Z

Thanks for updates!

Do you want to do that on the client-side, right in the browser?

I see what you mean - our existing impor function indeed fetches from URL and parse JSON right in the browser, but I do not think that such architecture will suit our case, as then the whole IPFS stack need to be run on the client. And it is not something that we want add here (we want self-contained IpfsNotebookRepo implementation)

I think what you could do here is - create a new REST API endpoint, pass this hash (which is like an URL in our case) to this REST API, and make it delegate actual fetching logic to a method in IpfsNotebookRepo and then, on the client, just rear the response, as a JSON.

This logic can be later used for BitTorrent notebooks storage support as well as simple URL case could be refactored to use it (but that is outside of the scope of this PR of course)

Let me introduce you to @corneadoug - he is known Zeppelin front-end ninja, whose knowledge of the client side codebase is by far greater than mine.

@corneadoug Could you please help @onkarshedge and guide him on refactoring\updating notenameImport.controller.js in such way, that it could delegate hash->JSON resolution in case of notebook import to the backend API, for the case of importing notes from new IPFS storage by IPFS hash (which is like a bittorent address)?

corneadoug · 2016-08-10T03:30:51Z

@onkarshedge just like @bzz said, it would be better to have that import function in the back-end.
This is the PR that first implemented the feature so you should be able to find where are the classes currently taking care of the import.

We currently have 2 options for import in the front-end: URL or File Import
You could probably create a 3rd one for IPFS.

Right now, the websocket call is sending a Notebook, so you might want to refactor that to have a choice between sending a Notebook or a URL/Hash/Other.
And send the Hash to the backend so it can do the import.

bzz · 2016-08-16T08:25:26Z

@onkarshedge what do you think?

onkarshedge · 2016-08-16T20:09:51Z

+ Future<Note> noteFuture = executorService.submit(task);
+ Note note = null;
+ try {
+ note = noteFuture.get(15, TimeUnit.SECONDS);


@bzz When the note is not available from peer it should not wait indefinitely so I added timeout in backend.
And the exception is logged. Should this be propagated to front-end ?
The front-end does not know about the TimeoutException.
@corneadoug should there be another timeout function in front-end ?

onkarshedge · 2016-08-16T20:10:00Z

@bzz @corneadoug please review,it is complete. I have added the screenshot.

corneadoug · 2016-08-17T02:11:37Z

@onkarshedge I would need a scenario/setup to be able to test it, could you add it to the PR description?
Regarding the timeout function in front-end, I think it would be better to send back a websocket event message with import result to that client (not everybody), and then show it in a ng-toast. (green: The Notebook XXX was imported, red: We couldn't import Notebook XXX)

bzz · 2016-08-17T05:43:05Z

@onkarshedge good progress, thank you for keeping us posted, your changes look very well!

Before moving further, on top of great suggestions from @corneadoug, there are few more things you also need to take care of:

Please make sure that zeppelin-distribution/src/bin_license/LICENSE includes all dependencies AND all transitive dependencies that this changes bring to the project. You can see them with mvn dependency:tree
Please make sure that ALL your Java code follows the conventions from project style guide
I noticed that CI is failing here. Could you, as an author of the PR, look into that and post there the reason of the failures here please? This would speed up the review and enable the reviewers to help you identify the reason faster.

Please ping back as soon as those are addressed and I will be happy to make another pass on it!

onkarshedge · 2016-08-17T06:07:07Z

@bzz there are no transitive dependencies, just Junit and hamcrest
The reason of CI failure is

[ERROR] Failed to execute goal on project zeppelin-zengine: Could not resolve dependencies for project org.apache.zeppelin:zeppelin-zengine:jar:0.7.0-SNAPSHOT: Failed to collect dependencies at org.ipfs:api:jar:0.0.1-SNAPSHOT: Failed to read artifact descriptor for org.ipfs:api:jar:0.0.1->SNAPSHOT: Could not transfer artifact org.ipfs:api:pom:0.0.1-SNAPSHOT from/to codehaus-snapshots (https://nexus.codehaus.org/snapshots/): nexus.codehaus.org: Name or service not known: Unknown host nexus.codehaus.org: Name or service not known -> [Help 1]

It has to be added in local .m2 repository

onkarshedge · 2016-08-18T06:33:26Z

@corneadoug feedback addressed.

onkarshedge · 2016-08-22T16:28:59Z

@bzz I found this documentation and there would be no need for dependency. I have used apache-httpcomponents httpclient.

onkarshedge · 2016-08-23T06:19:16Z

@bzz CI is green please review.

bzz · 2016-08-24T02:48:36Z

+ newNote.addCloneParagraph(p);
+ }
+ newNote.persist(subject);
+ } catch (IOException e) {


Instead of e.toString() we could have something like "Importing note from " + url+ " failed". The error message will still be there, as we propaget e.

+1 for having more detailed error message as mentioned by @bzz

I referred the previous import function.

bzz · 2016-08-24T03:02:41Z

Looks great, @onkarshedge! I posted a few comments above.

\cc @khalidhuseynov for extra back-end code review and @corneadoug for front-end one

corneadoug · 2016-08-24T05:45:50Z

 angular.element('#noteImportModal').modal('hide');
 });
+
+ $scope.$on('importNoteFail', function(event, data) {


'importNoteFail' isn't a great name, how about 'importNoteResult'

corneadoug · 2016-08-24T06:57:02Z

I'm a bit lost with this feature, is it really considered as a NotebookRepo or does it only keep the revisions? Does importing the Hash mean that it import a Revision as Notebook?

Why do we need to comment the VFS while it still save in local file system?

Also I think the ipfs installation steps shouldn't be in the NotebookRepo doc since not every user should be having a server running. It would be more fitting in this PR description on how to test this PR.

Import form

I would have classic URL by default, and the hash option only as a checkbox.
The Notebook name during import is also not used

Import Note event listener

I left comments in the code

corneadoug · 2016-08-24T07:00:24Z

Considering #1304 is the import really needed?

khalidhuseynov · 2016-08-24T07:15:30Z

in a sense i agree with @corneadoug and i'll write more details on that later, but for the beginning wouldn't it make sense to save normal notes without commit in ipfs as well?

backend and frontend added import status as ng-toast Added documentation

…dio to checkbox, notename is used

onkarshedge · 2016-08-30T12:32:05Z

@bzz @corneadoug addressed the feedback, updated PR description and also images.
@khalidhuseynov saving normal notes would create an ipfs object for each save , we would have to discard the previous object and hash and will need to store the latest hash. It's similar to git, it would not be suitable to commit on every small change as save() function is called on timeout.

bzz · 2016-11-09T04:40:34Z

@onkarshedge thank you for the great work!

How do you think, how hard it would be to rebase it on latest master?

close#83close#86close#125close#133close#139close#146close#193close#203close#246close#262close#264close#273close#291close#299close#320close#347close#389close#413close#423close#543close#560close#658close#670close#728close#765close#777close#782close#783close#812close#822close#841close#843close#878close#884close#918close#989close#1076close#1135close#1187close#1231close#1304close#1316close#1361close#1385close#1390close#1414close#1422close#1425close#1447close#1458close#1466close#1485close#1492close#1495close#1497close#1536close#1545close#1561close#1577close#1600close#1603close#1678close#1695close#1739close#1748close#1765close#1767close#1776close#1783close#1799

jongyoul reviewed Jun 10, 2016
View reviewed changes

onkarshedge force-pushed the ipfs branch from d4eb14b to cb004efCompare June 18, 2016 11:36

onkarshedge force-pushed the ipfs branch from cb004ef to 4e6bf9cCompare June 21, 2016 06:14

onkarshedge force-pushed the ipfs branch from 4e6bf9c to 41e5432Compare August 4, 2016 07:24

onkarshedge force-pushed the ipfs branch from 41e5432 to 9b8df2eCompare August 16, 2016 10:09

bzz mentioned this pull request Aug 16, 2016
[Zeppelin - 683][WIP] BittorrentNotebookRepo #1231
Closed
1 task

onkarshedge force-pushed the ipfs branch from 9b8df2e to ffd784fCompare August 16, 2016 19:55

onkarshedge reviewed Aug 16, 2016
View reviewed changes

onkarshedge changed the title ~~Added ipfsnotebookrepo [ Zeppelin-683 WIP ]~~ Added ipfsnotebookrepo [ Zeppelin-683 ]Aug 18, 2016

onkarshedge closed this Aug 23, 2016

onkarshedge reopened this Aug 23, 2016

bzz reviewed Aug 24, 2016
View reviewed changes

corneadoug reviewed Aug 24, 2016
View reviewed changes

onkarshedgeand others added 3 commits August 30, 2016 17:28

Added ipfsnotebookrepo with Versioning and import feature
9e39e53
backend and frontend added import status as ng-toast Added documentation

refactored code to remove any dependencies and use httpclient
1f13f96

Refactored according to codestyles, changed message format,changed ra…
1918565
…dio to checkbox, notename is used

onkarshedge force-pushed the ipfs branch from b09876d to 1918565Compare August 30, 2016 12:23

asfgit closed this in c38a0a0May 9, 2018

Uh oh!

Conversation

onkarshedge commented Jun 10, 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!

onkarshedge commented Jun 10, 2016

Uh oh!

jongyoulJun 10, 2016

Choose a reason for hiding this comment

Uh oh!

onkarshedgeJun 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!

onkarshedgeJun 11, 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!

bzzJun 15, 2016

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

onkarshedge commented Jun 15, 2016

Uh oh!

onkarshedge commented Jun 15, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

onkarshedge commented Jun 20, 2016

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

bzz commented Jul 12, 2016

Uh oh!

onkarshedge commented Jul 12, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

onkarshedge commented Aug 4, 2016

Uh oh!

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

Uh oh!

Uh oh!

corneadoug commented Aug 10, 2016

Uh oh!

bzz commented Aug 16, 2016

Uh oh!

onkarshedgeAug 16, 2016

Choose a reason for hiding this comment

Uh oh!

onkarshedge commented Aug 16, 2016

Uh oh!

corneadoug commented Aug 17, 2016

Uh oh!

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

Uh oh!

Uh oh!

onkarshedge commented Aug 17, 2016

Uh oh!

onkarshedge commented Aug 18, 2016

onkarshedge commented Jun 10, 2016•
edited
Loading

onkarshedgeJun 10, 2016•
edited
Loading

onkarshedgeJun 11, 2016•
edited
Loading

bzz commented Jun 15, 2016•
edited
Loading

onkarshedge commented Jun 15, 2016•
edited
Loading

bzz commented Jun 20, 2016•
edited
Loading

bzz commented Jun 21, 2016•
edited
Loading

bzz commented Jul 7, 2016•
edited
Loading

onkarshedge commented Jul 12, 2016•
edited
Loading

bzz commented Jul 14, 2016•
edited
Loading

bzz commented Aug 3, 2016•
edited
Loading

bzz commented Aug 9, 2016•
edited
Loading

bzz commented Aug 17, 2016•
edited
Loading

corneadoug commented Aug 24, 2016•
edited
Loading