[GitHub] zeppelin issue #1007: Update and refactor NotebookRepo versioning API
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1007: Update and refactor NotebookRepo versioning API
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1007: Update and refactor NotebookRepo versioning API
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1007: Update and refactor NotebookRepo versioning API
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
