You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by corneadoug <gi...@git.apache.org> on 2016/08/09 05:28:51 UTC
[GitHub] zeppelin pull request #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
GitHub user corneadoug opened a pull request:
https://github.com/apache/zeppelin/pull/1304
[ZEPPELIN-1190] [WIP] Visit Notebook Revision
### 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
* [ ] - Click on revision action
* [ ] - Select the right revision in the dropdown
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1190
### How should this be tested?
No test yet
### 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)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/corneadoug/incubator-zeppelin feature/visitRevision
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/1304.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1304
----
commit 8512381d5ee3f0ff7aee3a28fe78104aece243fc
Author: Damien CORNEAU <co...@gmail.com>
Date: 2016-07-29T06:23:18Z
create new root and call revision
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1304
this have been stale for quite a while, so I'll start looking into it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1304
@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);](https://github.com/apache/zeppelin/blob/3bd94ddc77ef9f9853ae978bf024d206378f16ba/zeppelin-web/src/components/websocketEvents/websocketMsg.service.js#L175)
And on the back-end is a [NOTE_REVISION](https://github.com/apache/zeppelin/blob/36a7e38ffd6af614ad770a6e23ec1fd98a90c809/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java#L239) 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()](https://github.com/apache/zeppelin/blob/5cdc02d3651fc8f5b7f9aa81f1d617007932b666/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java#L366) API, both, function name AND a signature, as that is the place where actual work of retrieving the Note is done, to something like:
```java
getNoteByRevisionId(String noteId, String revisionId, AuthenticationInfo subject)
```
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](https://github.com/apache/zeppelin/pull/1254/files#diff-eea417ea15b9480afaf52ebe1f51449dR92) interface and follow your design from #1254 (and close #1277).
What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the issue:
https://github.com/apache/zeppelin/pull/1304
@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:
<img width="888" alt="screen shot 2016-08-30 at 3 13 14 pm" src="https://cloud.githubusercontent.com/assets/710411/18079516/2ddd7944-6ecc-11e6-9be3-632dc0a1a704.png">
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the issue:
https://github.com/apache/zeppelin/pull/1304
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1304
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1304
@corneadoug thanks for letting me know, i'll look into that and get back to you.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1304
@corneadoug thanks for taking care of this! #1308 is merged and instead of `RevisionId` just send `Revision`. rebase should solve the problems.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1304
To avoid the confusion, could you guys 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 REST API structure which are not met? Which REST API endpoint are we talking about and what exactly does not work now?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug closed the pull request at:
https://github.com/apache/zeppelin/pull/1304
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1304
any ETA on this one, so that can plan accordingly?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1304
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by corneadoug <gi...@git.apache.org>.
GitHub user corneadoug reopened a pull request:
https://github.com/apache/zeppelin/pull/1304
[ZEPPELIN-1190] [WIP] Visit Notebook Revision
### 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
* [x] - API needs to be fixed to send Revision ID
* [ ] - Render Revision
* [ ] - Click on revision action
* [ ] - Select the right revision in the dropdown
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1190
### How should this be tested?
No test yet
### 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)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/corneadoug/incubator-zeppelin feature/visitRevision
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/1304.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1304
----
commit e5f28e938587601e78484e5a1eb239803b9c160a
Author: Damien CORNEAU <co...@gmail.com>
Date: 2016-07-29T06:23:18Z
create new root and call revision
commit 8d5deeee1f3275938258fb4a992221c93722185c
Author: Damien CORNEAU <co...@gmail.com>
Date: 2016-08-17T05:50:52Z
Change call to websocketEvent
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by anthonycorbacho <gi...@git.apache.org>.
Github user anthonycorbacho commented on the issue:
https://github.com/apache/zeppelin/pull/1304
@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 (UID) 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`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1304
I think this can be closed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1304
@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](https://github.com/apache/zeppelin/pull/1254#issuecomment-236475181)
>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](https://github.com/apache/zeppelin/pull/1254#issuecomment-236570441)
>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 :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1304
@corneadoug please let us know if there still are any troubles, after #1254 got merged!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/1304
---
[GitHub] zeppelin issue #1304: [ZEPPELIN-1190] [WIP] Visit Notebook Revision
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the issue:
https://github.com/apache/zeppelin/pull/1304
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---