[ZEPPELIN-1257] storage - fix get note revision api

[khalidhuseynov]

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

• - change storage api
• - modify notebook server

What is the Jira issue?

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

[khalidhuseynov]

/cc @corneadoug @bzz for review

[corneadoug]

@khalidhuseynov The CI is fully red

[bzz]

@khalidhuseynov care to elaborate on CI failure reason? (preferably, before the review)

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)

Right now I'm under impression that narrowing public API of NotebokRepo to String vs generic Revision interface that any client is free to implement is not something very usefull or generic enough.

I would prefer no making such changes to the public API for merele the sake of change, without a strong reason (which might very well exist but may be I'm missing here).

[khalidhuseynov]

@bzz CI failure was due to test, fixed now.

Further, regarding the flexibility and narrowing scope. Revision class is implemented here 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?

[bzz]

@khalidhuseynov glad to hear. But CI is red again, could you 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 what you think.

[khalidhuseynov]

@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

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.

[bzz]

@khalidhuseynov thank you!
Do you suggest merging this guy now and taking care of Revision separation later?

[khalidhuseynov]

@bzz i believe we decided not to merge it for now. I'll revise it and let you know

[bzz]

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?

[bzz]

@khalidhuseynov git rebase master to re-trigger CI?

[khalidhuseynov]

@bzz rebased and addressed comments

[khalidhuseynov]

ping

[bzz]

Thank you for friendly ping. Will review and get back to you ASAP

[bzz]

Do you think we it's better to keep this and to change to getNoteByRevision ?

[bzz]

@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

[khalidhuseynov]

@bzz right, addressed in a8d005b

[bzz]

Great! Merging to master, if there is no further discussion.

[anthonycorbacho]

@bzz can you merge it asap? i need this change if i want to continue my work