[ZEPPELIN-1190] [WIP] Visit Notebook Revision

[corneadoug]

What is this PR for?

A few sentences describing the overall goals of the pull request's commits.
First time? Check out the contributing guide - https://github.com/apache/zeppelin/blob/master/CONTRIBUTING.md

What type of PR is it?

To visit the Notebook Revision after selecting it from the dropdown

Todos

• - API needs to be fixed to send Revision ID
• - Render Revision
• - Visit a Revision using dropdown
• - Add Head in the revision list
• - Select the right revision in the dropdown
• - Make revision Read-Only
• - Block Websocket in Revision
• - Restyle the dropdown content

What is the Jira issue?

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

How should this be tested?

You need to add this config in conf/zeppelin-env.sh
export ZEPPELIN_NOTEBOOK_STORAGE="org.apache.zeppelin.notebook.repo.GitNotebookRepo"

Questions:

• Does the licenses files need update? No
• Is there breaking changes for older versions? No
• Does this needs documentation? No (until full feature is implemented)

[khalidhuseynov]

@corneadoug thanks for taking care of this! #1308 is merged and instead of RevisionId just send Revision. rebase should solve the problems.

[corneadoug]

@khalidhuseynov that doesn't solve anything, I can't work with Revision, it needs to be RevisionId.
If you visit a URL like: /notebook/:noteId/revision/:revisionId

All you have is the revisitionId and not the other informations, which means I can't make a call to get the Notebook content unless I get the list of Revisions first.

[khalidhuseynov]

@corneadoug thanks for clarification. in this case, how do you think it would look like if revisionId cannot be represented as simple string and it's more like complex object?

I believe @bzz had this point in the comments here

Also, could you please explain why do you think this API change is valuable and what benefits does it bring for a user? (i.e does it consider other possible versioned storage i.e IPFS, BitTorrent and other future ones, where just an ID might not be enough? or it might be not String)

and here

Revision class implementation that you point out looks good to me - it represents clearly defined interface with it's own responsibilities. But String does the contrary. May be this class is even big enough to deserve his own file. And other clients may choose to replace it with their own implementations. I would understand if you suggest to extract it to the interface - this will bring even more flexibility (but increase verbosity).

But replacing it with the String does not look good to me.

So let's nail down this question here, and maybe together in the same thread, we can come up with much better solution :)

[bzz]

Great point @khalidhuseynov ! Let's do exactly as you say, as this may be a good overriding argument in our previous design discussions that you mention.

To avoid the confusion, could you guys please quickly bring me up to speed with, what API needs to be fixed to send Revision ID means in this context?

As far as I understood, we are talking in this PR about adding a new page on the frontend under URL /notebook/:noteId/revision/:revisionId and in order to be able to do that, there are some some expectations about backend REST API structure which are not met? Which REST API endpoint are we talking about and what exactly does not work now?

[corneadoug]

@bzz

The front-end needs to be able to get the Notebook's revision and show it. To do that, there is a websocket event that originally looked like that: websocketMsgSrv.getNoteRevision($routeParams.noteId, $routeParams.revisionId);

After using it, it seemed that this websocket event was not well defined on front-end side and that instead of revisionId I should be sending the whole revision description object to the backend.

Which look like that:

revision: {
timestamp,
text,
revisionId
}

Now, unless there is a whole object to save, there should not be any reason for the front-end to send a full object to the back-end so that it can understand which revision I want. That's precisely what revisionId should be for, especially if I want to have a custom URL that allows loading that revision.

Furthermore, in order for me to send a full revision description object, it would mean that I need to get the list of Revisions before I'm able to make any call to load the Notebook revision.

[anthonycorbacho]

@khalidhuseynov are we really sure about sending complex object as a parameter in the URL?
How will it be represented? encoded?

IMO it is not a good idea, if you ask for a resource, you need to call it by his Unique Identifier (A unique identifier (ID) is a numeric or alphanumeric string that is associated with a single entity within a given system. IDs make it possible to address that entity, so that it can be accessed and interacted with).

I never saw in my hole life a complex object as ID, but i can be wrong (i dont use hipster api) but to me it looks like it is not the way to go. I would rather have a simple String/Int as a ID than a weird object, like we have for notebook, you call it with is Id eg: /note/2AXXXX.

[bzz]

@corneadoug thank you for explaining!

So we are talking about WS API to fetch full notebook state from the history by it's revision? Wich on client-side looks like websocketMsgSrv.getNoteRevision($routeParams.noteId, $routeParams.revisionId);

And on the back-end is a NOTE_REVISION event in NotebookServer.

@khalidhuseynov I think first of all, there is a bit of confusion here: this event is handled by
getNoteRevision which looks more like getNoteByRevision because Revision, as discussed before, just represents the reference to the point in time, knowing which a full Note can be obtained, as it was back then. So getNoteRevision may be interpreted as a getter for Revision, rather than the Note.

Then, it makes perfect sense to change the Notebook.getNoteRevision() API, both, function name AND a signature, as that is the place where actual work of retrieving the Note is done, to something like:

getNoteByRevisionId(StringnoteId, StringrevisionId, AuthenticationInfosubject)

And this necessity, that comes from API client, at least for me, is an overriding argument in our discussion: you were right and we should adjust NotebookRepo.get interface and follow your design from #1254 (and close #1277).

What do you think?

[khalidhuseynov]

@bzz exactly, that's the point i was trying to make in #1254.
@anthonycorbacho i'm completely agree with you, and that was the original starting premise.
I'm happy that everyone got the point now! I'll reopen and rebase #1254 then.

[bzz]

@corneadoug please let us know if there still are any troubles, after #1254 got merged!

[khalidhuseynov]

any ETA on this one, so that can plan accordingly?

[corneadoug]

@khalidhuseynov
I found out that websocket events at Notebook level are already blocked in the revision (I don't receive anything, and my actions are not affecting the original note).

However there is some errors on Zeppelin server:

To reproduce, you can visit a revision form the dropdown. Then try running a paragraph (for example), this happen for all the Notebook websocket event (Run, Save etc...)

The backend seems to be using a NotebookSocket param that isn't initiated in our case (which is good). However since the function are not checking the parameter, it generates a NPE

[khalidhuseynov]

@corneadoug thanks for letting me know, i'll look into that and get back to you.

[khalidhuseynov]

could you change it here to single quotes,jscs complaining

[khalidhuseynov]

this and below line as well

[khalidhuseynov]

@corneadoug I've just tried the scenario you have, and it was running correctly (without np) when you switch to different revisions. the only thing is that it was running the head version which is expected behaviour. so could you check it out again? possibly that np error was due to different problem.

[khalidhuseynov]

this have been stale for quite a while, so I'll start looking into it

[khalidhuseynov]

I think this can be closed