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/07/31 07:44:51 UTC
[GitHub] zeppelin pull request #1254: [ZEPPELIN-1257] storage - fix get note revision...
GitHub user khalidhuseynov opened a pull request:
https://github.com/apache/zeppelin/pull/1254
[ZEPPELIN-1257] storage - fix get note revision api
### What is this PR for?
Getting revision of note requires only unique revision id and the whole revision object isn't required. This PR is to fix the issue.
### What type of PR is it?
Bug Fix | Improvement
### Todos
* [x] - change storage api
* [x] - modify notebook server
### What is the Jira issue?
[ZEPPELIN-1257](https://issues.apache.org/jira/browse/ZEPPELIN-1257)
### How should this be tested?
Storage layer tests should pass, green CI
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* 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 fix/get-note-revision-api
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/1254.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 #1254
----
commit 96490a29295221a2dbc8f8bac50b5f87cf9bb847
Author: Khalid Huseynov <kh...@gmail.com>
Date: 2016-07-31T07:15:08Z
change api Revision -> String (revision id)
commit 3b7868e254bfe338755ebf4a9de8a71ac49db8b8
Author: Khalid Huseynov <kh...@gmail.com>
Date: 2016-07-31T07:31:39Z
change Revision -> String in NotebookServer
commit 1bab6a4c9b3bbca958c540c6c6cfd4e012504270
Author: Khalid Huseynov <kh...@gmail.com>
Date: 2016-07-31T07:37:46Z
consistency check: NotebookRepo.Revision -> 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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@khalidhuseynov thank you!
Do you suggest merging this guy now and taking care of Revision separation later?
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@bzz regarding the CI issue, I keep getting the following:
```
14:05:27,226 ERROR org.apache.zeppelin.interpreter.remote.RemoteInterpreter:237 - Failed to create interpreter: org.apache.zeppelin.interpreter.remote.mock.MockInterpreterA
14:05:27,227 ERROR org.apache.zeppelin.interpreter.remote.RemoteInterpreter:264 - Failed to initialize interpreter: org.apache.zeppelin.interpreter.remote.mock.MockInterpreterA. Remove it from interpreterGroup
14:05:27,240 INFO org.apache.zeppelin.scheduler.SchedulerFactory:131 - Job jobName1 started by scheduler test
14:05:27,240 INFO org.apache.zeppelin.interpreter.remote.RemoteInterpreter:223 - Create remote interpreter org.apache.zeppelin.interpreter.remote.mock.MockInterpreterA
14:05:27,242 ERROR org.apache.zeppelin.interpreter.remote.RemoteInterpreter:237 - Failed to create interpreter: org.apache.zeppelin.interpreter.remote.mock.MockInterpreterA
14:05:27,243 ERROR org.apache.zeppelin.scheduler.Job:189 - Job failed
org.apache.zeppelin.interpreter.InterpreterException: org.apache.thrift.TApplicationException: Internal error processing createInterpreter
at org.apache.zeppelin.interpreter.remote.RemoteInterpreter.init(RemoteInterpreter.java:238)
at org.apache.zeppelin.interpreter.remote.RemoteInterpreter.getFormType(RemoteInterpreter.java:383)
at org.apache.zeppelin.interpreter.remote.RemoteInterpreter.interpret(RemoteInterpreter.java:299)
at org.apache.zeppelin.scheduler.RemoteSchedulerTest$2.jobRun(RemoteSchedulerTest.java:210)
at org.apache.zeppelin.scheduler.Job.run(Job.java:176)
at org.apache.zeppelin.scheduler.RemoteScheduler$JobRunner.run(RemoteScheduler.java:329)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
at java.util.concurrent.FutureTask.run(FutureTask.java:262)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:178)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:292)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
at java.lang.Thread.run(Thread.java:745)
Caused by: org.apache.thrift.TApplicationException: Internal error processing createInterpreter
at org.apache.thrift.TApplicationException.read(TApplicationException.java:111)
at org.apache.thrift.TServiceClient.receiveBase(TServiceClient.java:71)
at org.apache.zeppelin.interpreter.thrift.RemoteInterpreterService$Client.recv_createInterpreter(RemoteInterpreterService.java:196)
at org.apache.zeppelin.interpreter.thrift.RemoteInterpreterService$Client.createInterpreter(RemoteInterpreterService.java:180)
at org.apache.zeppelin.interpreter.remote.RemoteInterpreter.init(RemoteInterpreter.java:227)
... 12 more
```
filed an issue [here](https://issues.apache.org/jira/browse/ZEPPELIN-1264)
Regarding this pr, I'll consider your comments on using OO way here and open new pr with a fix. Also separating Revision is a valid point and might be addressed there.
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1254
Got it! Thank you for clarification.
Would you mind closing it for now and then re-opening it as soon as it is 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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1254
/cc @corneadoug @bzz 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 pull request #1254: [ZEPPELIN-1257] storage - fix get note revision...
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov closed the pull request at:
https://github.com/apache/zeppelin/pull/1254
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@khalidhuseynov glad to hear. But CI is red again, could you, as an author of PR please post here the reason of failure? This will save time for the reviewers.
And thank you for kind for the explanation!
> The point of this PR was not to send unnecessary info in the Revision object
This is exactly what I meant by "changes for the sake of changes".
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.
After some thinking - may be I lack verbal skills to convey the message about obejct-oriented design approach. If that is the case, please check `Item 50: Avoid strings where other types are more appropriate`, page 224 of "Effective Java" 2nd edition and let me know.
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision...
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov closed the pull request at:
https://github.com/apache/zeppelin/pull/1254
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@khalidhuseynov The CI is fully red
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by anthonycorbacho <gi...@git.apache.org>.
Github user anthonycorbacho commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@bzz can you merge it asap? i need this change if i want to continue my work
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1254
Thank you for friendly ping. Will review and get back to you ASAP
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@khalidhuseynov CI is green and changes looks great to me, modulo one naming case noted above.
Please let me know if you want to address that one as well. After that I will be happy to mered it to master, if there is no further 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 issue #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1254
Great! Merging to master, if there is no further 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 issue #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@bzz rebased and addressed comments
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@khalidhuseynov could you please elaborate on CI failure reason?
Also, could you please explain why do you think this is valuable change and what benefits does it bring?
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@bzz i believe we decided not to merge it for now. I'll revise it and let you know
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@khalidhuseynov `git rebase master` to re-trigger CI?
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/1254
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision...
Posted by khalidhuseynov <gi...@git.apache.org>.
GitHub user khalidhuseynov reopened a pull request:
https://github.com/apache/zeppelin/pull/1254
[ZEPPELIN-1257] storage - fix get note revision api
### What is this PR for?
Getting revision of note requires only unique revision id and the whole revision object isn't required. This PR is to fix the issue.
### What type of PR is it?
Bug Fix | Improvement
### Todos
* [x] - change storage api
* [x] - modify notebook server
### What is the Jira issue?
[ZEPPELIN-1257](https://issues.apache.org/jira/browse/ZEPPELIN-1257)
### How should this be tested?
Storage layer tests should pass, green CI
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* 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 fix/get-note-revision-api
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/1254.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 #1254
----
commit 96490a29295221a2dbc8f8bac50b5f87cf9bb847
Author: Khalid Huseynov <kh...@gmail.com>
Date: 2016-07-31T07:15:08Z
change api Revision -> String (revision id)
commit 3b7868e254bfe338755ebf4a9de8a71ac49db8b8
Author: Khalid Huseynov <kh...@gmail.com>
Date: 2016-07-31T07:31:39Z
change Revision -> String in NotebookServer
commit 1bab6a4c9b3bbca958c540c6c6cfd4e012504270
Author: Khalid Huseynov <kh...@gmail.com>
Date: 2016-07-31T07:37:46Z
consistency check: NotebookRepo.Revision -> Revision
commit f14c15afb59da0e2c7bdab0cd443653276cdbbfc
Author: Khalid Huseynov <kh...@gmail.com>
Date: 2016-08-01T04:59:16Z
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@bzz right, addressed in a8d005b70c65fe68f2098b4460b2e699fd681b7f
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1254
ping
---
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 #1254: [ZEPPELIN-1257] storage - fix get note revision...
Posted by khalidhuseynov <gi...@git.apache.org>.
GitHub user khalidhuseynov reopened a pull request:
https://github.com/apache/zeppelin/pull/1254
[ZEPPELIN-1257] storage - fix get note revision api
### What is this PR for?
Getting revision of note requires only unique revision id and the whole revision object isn't required. This PR is to fix the issue.
### What type of PR is it?
Bug Fix | Improvement
### Todos
* [x] - change storage api
* [x] - modify notebook server
### What is the Jira issue?
[ZEPPELIN-1257](https://issues.apache.org/jira/browse/ZEPPELIN-1257)
### How should this be tested?
Storage layer tests should pass, green CI
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* 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 fix/get-note-revision-api
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/1254.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 #1254
----
commit 96490a29295221a2dbc8f8bac50b5f87cf9bb847
Author: Khalid Huseynov <kh...@gmail.com>
Date: 2016-07-31T07:15:08Z
change api Revision -> String (revision id)
commit 3b7868e254bfe338755ebf4a9de8a71ac49db8b8
Author: Khalid Huseynov <kh...@gmail.com>
Date: 2016-07-31T07:31:39Z
change Revision -> String in NotebookServer
commit 1bab6a4c9b3bbca958c540c6c6cfd4e012504270
Author: Khalid Huseynov <kh...@gmail.com>
Date: 2016-07-31T07:37:46Z
consistency check: NotebookRepo.Revision -> Revision
commit f14c15afb59da0e2c7bdab0cd443653276cdbbfc
Author: Khalid Huseynov <kh...@gmail.com>
Date: 2016-08-01T04:59:16Z
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 #1254: [ZEPPELIN-1257] storage - fix get note revision api
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/1254
@bzz CI failure was due to test, fixed now.
Further, regarding the flexibility and narrowing scope. Revision class is implemented [here](https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepo.java#L106) and as you can see currently when implementing your own NotebookRepo you reuse same class (without implementing your own one). Implementing your own revision class is a good idea which is not supported yet (it should be handled properly in frontend as well).
The point of this PR was not to send unnecessary info in the Revision object since only thing to identify the revision is supposed to be its revisionId and it being String seems at least intuitive since noteId, and paragraphId are identified in the same way by String id. So having noteId, revisionId should uniquely identify the revision. Can you give some example of ID that's more than one element and not presented as string?
---
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.
---