You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by khalidhuseynov <gi...@git.apache.org> on 2016/06/13 22:31:52 UTC
[GitHub] zeppelin pull request #1007: Update and refactor NotebookRepo versioning API
GitHub user khalidhuseynov opened a pull request:
https://github.com/apache/zeppelin/pull/1007
Update and refactor NotebookRepo versioning API
### What is this PR for?
This is firstly to refactor API for versioning and keep everthing inside of one interface (NotebookRepo) instead of two different interfaces (NotebookRepoVersioned). Secondly, there're modifications to existing versioning api, with considerations of future complete implementation of versioning. Note that this PR doesn't implement all suggested interfaces, but lays foundation for their implementation.
### What type of PR is it?
Improvement && Refactoring
### Todos
* [x] - move versioning api (get, history) from NotebookRepoVersioned to NotebookRepo
* [x] - refactor and naming changes
* [x] - modify checkpoint api (add return value) and modify NotebookRepoSync to deal with it
* [ ] - address comments
### What is the Jira issue?
### How should this be tested?
Basically it doesn't add new functionality, so the only requirement is for tests to pass.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? not breaking, but some api changes
* Does this needs documentation? No
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/khalidhuseynov/incubator-zeppelin repo/versioning-api-update
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/1007.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 #1007
----
commit faf1b5b06a0753fa728caec7579f1bb6118137ae
Author: Khalid Huseynov <kh...@nflabs.com>
Date: 2016-06-13T22:10:03Z
move and update versioning api
first of all versioning api was moved into NotebookRepo to keep all api in one place, secondly it was updated with Rev class referring to revision unit
commit 3dce38bcd4b7a1eeb110e336d069527028a7429c
Author: Khalid Huseynov <kh...@nflabs.com>
Date: 2016-06-13T22:14:12Z
propagate changes to all repos
commit 1ec67e648afb39cbc50ba1b98824c8a86e70828a
Author: Khalid Huseynov <kh...@nflabs.com>
Date: 2016-06-13T22:14:46Z
apply changes to NotebookRepoSync
commit ee1eeb3357af4a051625d7058819c7e3aad3be8d
Author: Khalid Huseynov <kh...@nflabs.com>
Date: 2016-06-13T22:15:24Z
fix tests
----
---
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 #1007: Update and refactor NotebookRepo versioning API
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1007
yeah, I agree that having two interfaces is more elegant, but let's see some more detailed downsides. if you see [here](https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java#L49) repoSync consists of `NotebookRepo` classes, thus when having any versioning related operation we will need to check everytime if repo instance is `instanceof NotebooRepoVersioned` (or some other flag) and then do cast and call corresponding method, which is an overhead @bzz @Leemoonsoo WDYT, any other opinions?
Moreover, `NotebookRepoVersioned` haven't been really integrated into the workflow so far, so it's good time to think on either integrating or removing it to move on. And the only existing versioning related `checkpoint` method was in `NotebookRepo` instead of `NotebookRepoVersioned`.
---
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 #1007: Update and refactor NotebookRepo versioning API
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1007
@bzz thanks for the review, I'll be glad to hear for the elegant solutions of the mentioned issues, even can help with implementation :)
---
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 #1007: Update and refactor NotebookRepo versioning API
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1007
the reason is quite simple, that's to have only one interface to be implemented (instead of choosing from two, given that documentation doesn't mention `NotebookRepoVersioned` so far). Also I believe that would encourage to implement versioning on more storages. @bzz 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 pull request #1007: Update and refactor NotebookRepo versioning API
Posted by khalidhuseynov <gi...@git.apache.org>.
GitHub user khalidhuseynov reopened a pull request:
https://github.com/apache/zeppelin/pull/1007
Update and refactor NotebookRepo versioning API
### What is this PR for?
This is firstly to refactor API for versioning and keep everthing inside of one interface (NotebookRepo) instead of two different interfaces (NotebookRepoVersioned). Secondly, there're modifications to existing versioning api, with considerations of future complete implementation of versioning. Note that this PR doesn't implement all suggested interfaces, but lays foundation for their implementation.
### What type of PR is it?
Improvement && Refactoring
### Todos
* [x] - move versioning api (get, history) from NotebookRepoVersioned to NotebookRepo
* [x] - refactor and naming changes
* [x] - modify checkpoint api (add return value) and modify NotebookRepoSync to deal with it
### What is the Jira issue?
### How should this be tested?
Basically it doesn't add new functionality, so the only requirement is for tests to pass.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? not breaking, but some api changes
* Does this needs documentation? No
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/khalidhuseynov/incubator-zeppelin repo/versioning-api-update
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/1007.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 #1007
----
commit faf1b5b06a0753fa728caec7579f1bb6118137ae
Author: Khalid Huseynov <kh...@nflabs.com>
Date: 2016-06-13T22:10:03Z
move and update versioning api
first of all versioning api was moved into NotebookRepo to keep all api in one place, secondly it was updated with Rev class referring to revision unit
commit 3dce38bcd4b7a1eeb110e336d069527028a7429c
Author: Khalid Huseynov <kh...@nflabs.com>
Date: 2016-06-13T22:14:12Z
propagate changes to all repos
commit 1ec67e648afb39cbc50ba1b98824c8a86e70828a
Author: Khalid Huseynov <kh...@nflabs.com>
Date: 2016-06-13T22:14:46Z
apply changes to NotebookRepoSync
commit ee1eeb3357af4a051625d7058819c7e3aad3be8d
Author: Khalid Huseynov <kh...@nflabs.com>
Date: 2016-06-13T22:15:24Z
fix tests
commit 6472888c7982d21710eb7dcea2aa9527d271ad35
Author: Khalid Huseynov <kh...@nflabs.com>
Date: 2016-06-13T23:18:55Z
fix checkstyle
commit 141f5df8dfd71199d9000281f1e9d1da1f7c3752
Author: Khalid Huseynov <kh...@nflabs.com>
Date: 2016-06-14T01:49:30Z
Merge branch 'master' into repo/versioning-api-update
commit f084b3fe1352f6569b936e6553ad4e2512731c62
Author: Khalid Huseynov <kh...@nflabs.com>
Date: 2016-06-14T18:18:22Z
Rev -> 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 #1007: Update and refactor NotebookRepo versioning API
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1007
ready for review
---
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 #1007: Update and refactor NotebookRepo versioning API
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1007
Good point, if asked, I would say that having `NotebookRepoSync implements NotebookRepo` is not an elegant design either - NotebookRepoSync is not a NoteboksRepo! :)
It's up to you after all, I see that you feel it's the best way to integrate for now and is as good as it can be - so let's keep it like this. It can always be refactored later on.
Looks good to me, will merge if there is no other discussion.
---
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 #1007: Update and refactor NotebookRepo versioning API
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/1007
---
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 #1007: Update and refactor NotebookRepo versioning API
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov closed the pull request at:
https://github.com/apache/zeppelin/pull/1007
---
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 #1007: Update and refactor NotebookRepo versioning API
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1007
Great improvement!
Could you please explain the rationale behind removing `NotebookRepoVersioned` and making a lot of boilerplate methods returning `null` in all other notebook storages that do not support versioning?
---
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 #1007: Update and refactor NotebookRepo versioning API
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1007
Thank you for addressing feedback promptly.
Well, in my oppinion, if by "encourage more versioned implementation" you mean having this code duplicated 5 times around the code base
```java
@Override
public Note get(String noteId, Revision rev) throws IOException {
// Auto-generated method stub
return null;
}
```
it looks like not very elegant solution.
It looks more meaningful and elegant to having 2 separate interfaces (types) documented, 1 for non-versioned backends and 1 for versioned ones to allow authors to choose from.
Otherwise, if we extend the suggested logic - why do not just to type everything as `Object` and keep only 1 interface for everything? It kind of defeats the benefits of having a type system.
It's not a strong opinion though, and if after this arguments, you still feel that having single one is better - please let me know.
Other that this issues - it looks great to me.
---
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.
---