[ZEPPELIN-982] Improve interpreter completion API #984

AhyoungRyu · 2016-06-10T02:18:02Z

What is this PR for?

When people implement a new interpreter, they extend interpreter.java as described in here. Among the several methods in interpreter.java, completion API enables auto-completion.

However this API is too simple compared to other project's auto-completion and hard to add more at the moment. So for the aspect of further expansion, it would be better to separate and restructure this API before the this release( 0.6.0 ).

What type of PR is it?

Improvement

Todos

- Create new structure : InterpreterCompletion in RemoteInterpreterService.thrift and regenerate zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/thrift/* files
- Change all existing List<String> completion -> List<InterpreterCompletion> completion
- Change paragraph.controller.js to point real name and value

What is the Jira issue?

ZEPPELIN-982

How should this be tested?

Since this improvement is just API change, it should work same as before. So after applying this patch, and check whether auto-completion works well or not.

Use . + ctrl for auto-completion. For example,

%spark 
sc.version

When after typing sc. and pushing . + ctrl down, version should be shown in the auto-completion list.

Screenshots (if appropriate)

Questions:

Does the licenses files need update? No
Is there breaking changes for older versions? No
Does this needs documentation? No

jongyoul · 2016-06-10T09:37:24Z

...nterpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java

@@ -416,10 +413,12 @@ public String getFormType(String noteId, String className) throws TException {
  }

  @Override
-  public List<String> completion(String noteId, String className, String buf, int cursor)
+  public List<InterpreterCompletion> completion(String noteId, String className, String buf,
+                                                int cursor)


Could you please turn off vertical alignment? If you are using Intellij, you can turn it off where 'Editor' -> 'Code Style' -> 'java' -> 'Wrapping and Braces' -> 'Method declaration parameters' -> 'Align when multiline'.

Yeah I did as you said and repushed. If I did wrong, please let me know :)
The reason why I did like this is this line is over 100 line length. So I cut this line on purpose.

jongyoul · 2016-06-10T09:43:23Z

@AhyoungRyu What the value of InterpreterCompletion means? And I suggest we remove the mark abstract from Interpreter class because almost interpreter returns empty list and it's better to offer that behaviour by default. How about it?

minahlee · 2016-06-10T17:36:30Z

...nterpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java

-  public List<InterpreterCompletion> completion(String noteId, String className, String buf,
-                                                int cursor)
+  public List<InterpreterCompletion> completion(String noteId,
+                                                String className, String buf, int cursor)


@AhyoungRyu Zeppelin follows google code style. See https://google.github.io/styleguide/javaguide.html#s4.5.2-line-wrapping-indent

AhyoungRyu · 2016-06-10T17:46:55Z

@jongyoul Makes sense. I addressed your feedback. Please see my last two commit :)

Regarding value,
as you can see, I used two variables(name and value) for InterpreterCompletion. When you use the auto-completion in the paragraph, you can see the several names in the suggested list. And if you select something in this list, the value that you choose will be returned. Actually these two variables are already existed in paragraph.controller.js. I just connected these two.

jongyoul · 2016-06-11T14:37:14Z

@AhyoungRyu I see. Thanks.

bzz · 2016-06-13T09:22:55Z

@AhyoungRyu there are 2 different test failures on CI here:

Integration test fails https://s3.amazonaws.com/archive.travis-ci.org/jobs/137185986/log.txt

Tests in error: 
  ZeppelinIT.testSparkInterpreterDependencyLoading:210 Â» NoSuchElement Unable to...

Caused by: org.openqa.selenium.NoSuchElementException: Unable to locate element: {"method":"xpath","selector":"//div[@class='modal-dialog'][contains(.,'Do you want to update this interpreter and restart with new settings?')]//div[@class='modal-footer']//button[contains(.,'OK')]"}

Cassandra interpreter test failure https://api.travis-ci.org/jobs/137185978/log.txt?deansi=true

.apache.zeppelin.cassandra.CassandraInterpreterTest  Time elapsed: 29.999 sec  <<< ERROR!
java.lang.ExceptionInInitializerError: null
    at com.datastax.driver.core.exceptions.NoHostAvailableException.copy(NoHostAvailableException.java:84)
    at com.datastax.driver.core.exceptions.NoHostAvailableException.copy(NoHostAvailableException.java:37)
    at com.datastax.driver.core.DriverThrowables.propagateCause(DriverThrowables.java:37)
    at com.datastax.driver.core.DefaultResultSetFuture.getUninterruptibly(DefaultResultSetFuture.java:245)
    at com.datastax.driver.core.AbstractSession.execute(AbstractSession.java:63)
    at info.archinnov.achilles.script.ScriptExecutor.executeScript(ScriptExecutor.java:61)
    at info.archinnov.achilles.embedded.AchillesInitializer.executeStartupScripts(AchillesInitializer.java:204)
    at info.archinnov.achilles.embedded.AchillesInitializer.initialize(AchillesInitializer.java:98)
    at info.archinnov.achilles.embedded.AchillesInitializer.initializeFromParameters(AchillesInitializer.java:61)
    at info.archinnov.achilles.embedded.CassandraEmbeddedServer.<init>(CassandraEmbeddedServer.java:76)
    at info.archinnov.achilles.embedded.CassandraEmbeddedServerBuilder.buildNativeSessionOnly(CassandraEmbeddedServerBuilder.java:436)
    at org.apache.zeppelin.cassandra.CassandraInterpreterTest.<clinit>(CassandraInterpreterTest.java:59)
Caused by: com.datastax.driver.core.exceptions.NoHostAvailableException: All host(s) tried for query failed (tried: localhost/127.0.0.1:9227

If you believe neither of them are related to this changes - the best way to handle it is to file a JIRA issues (with the name of the test failing in the title), label flaky-test and attach the links to CI logs.

bzz · 2016-06-13T09:26:17Z

zeppelin-interpreter/src/main/thrift/RemoteInterpreterService.thrift

@@ -56,6 +56,11 @@ struct RemoteInterpreterEvent {
  2: string data      // json serialized data
 }

+struct InterpreterCompletion {
+  1: string name,
+  2: string value


They PR is huge so sorry if I have missed it, but could you please point to the place where this API is used?
Right now, looking at this it is not clear what exactly is name and value here

@bzz Regarding the two value name and value, can this comment be an answer for your question ?

Do you think we should add a link to this comment inside the source code then?
Because the next person reading it will have exactly the same question as two others had here before.

Or how do you think, is there a way we could improve things here, without referencing legacy naming from the frontend code?

I.e ones the come to my mind and seems reasonable for a completion suggestion to have are:

text and code or

visible_suggestion and source_code

etc

AhyoungRyu · 2016-06-13T09:30:16Z

@bzz Yeah thanks for pointing it. I'll take a look at it and the conflictions too.

AhyoungRyu · 2016-06-13T17:54:17Z

zeppelin-interpreter/src/main/thrift/RemoteInterpreterService.thrift

+  1: string name,
+  2: string value
+}
+


@bzz I added some descriptions instead of changing overall variable names. Since name & value is already existed variable name in paragraph.controll.js.

Leemoonsoo · 2016-06-15T20:32:34Z

Thanks @AhyoungRyu for the improvement and taking care of comments.

LGTM. Merge it into master and 0.6 branch if there're no more discussions.

@minalee

### What is this PR for? When people implement a new interpreter, they extend [interpreter.java](https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/Interpreter.java) as described in [here](https://zeppelin.apache.org/docs/0.6.0-SNAPSHOT/development/writingzeppelininterpreter.html). Among the several methods in [interpreter.java](https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/Interpreter.java), [completion API](https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/Interpreter.java#L109) enables auto-completion. However this API is too simple compared to other project's auto-completion and hard to add more at the moment. So for the aspect of further expansion, it would be better to separate and restructure this API before the this release( 0.6.0 ). ### What type of PR is it? Improvement ### Todos * [x] - Create new structure : `InterpreterCompletion` in `RemoteInterpreterService.thrift` and regenerate `zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/thrift/*` files * [x] - Change all existing `List<String> completion` -> `List<InterpreterCompletion> completion` * [x] - Change `paragraph.controller.js` to point real `name` and `value` ### What is the Jira issue? [ZEPPELIN-982](https://issues.apache.org/jira/browse/ZEPPELIN-982) ### How should this be tested? Since this improvement is just API change, it should work same as before. So after applying this patch, and check whether auto-completion works well or not. Use `. + ctrl` for auto-completion. For example, ``` %spark sc.version ``` When after typing `sc.` and pushing `. + ctrl` down, `version` should be shown in the auto-completion list. ### Screenshots (if appropriate) ![auto_completion](https://cloud.githubusercontent.com/assets/10060731/15952521/72937782-2e76-11e6-8246-4faf0dd77a5b.gif) ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: AhyoungRyu <[email protected]> Closes #984 from AhyoungRyu/ZEPPELIN-982 and squashes the following commits: 311dc29 [AhyoungRyu] Fix travis 9d384ec [AhyoungRyu] Address @minalee feedback fdfae8f [AhyoungRyu] Address @jongyoul review bd4f8c0 [AhyoungRyu] Remove abstract and make it return null by default f8352c7 [AhyoungRyu] Fix travis error 43d81f6 [AhyoungRyu] Remove console.log 24912fa [AhyoungRyu] Fix type casting error in SparkInterpreter 80e295b [AhyoungRyu] Change return type bd04c22 [AhyoungRyu] Apply new InterpreterCompletion class to all interpreter class files c283043 [AhyoungRyu] Apply new InterpreterCompletion class under zeppelin-zengine/ dbecc51 [AhyoungRyu] Apply new InterpreterCompletion class under zeppelin-server/ 6449455 [AhyoungRyu] Apply new InterpreterCompletion class under zeppelin-interpreter/ 919b159 [AhyoungRyu] Add automatically generated thrift class 9e69e11 [AhyoungRyu] Change v -> v.name & v.value in front 73e374e [AhyoungRyu] Define InterpreterCompletion structure to thrift file (cherry picked from commit 7b00dff) Signed-off-by: Lee moon soo <[email protected]>

AhyoungRyu closed this Jun 10, 2016

AhyoungRyu reopened this Jun 10, 2016

jongyoul reviewed Jun 10, 2016
View reviewed changes

minahlee reviewed Jun 10, 2016
View reviewed changes

AhyoungRyu force-pushed the ZEPPELIN-982 branch from a60d0f9 to 3c6fd26 Compare June 12, 2016 18:33

AhyoungRyu closed this Jun 12, 2016

AhyoungRyu reopened this Jun 12, 2016

AhyoungRyu force-pushed the ZEPPELIN-982 branch from 8bbc3ec to 364368e Compare June 13, 2016 08:02

AhyoungRyu closed this Jun 13, 2016

AhyoungRyu reopened this Jun 13, 2016

bzz reviewed Jun 13, 2016
View reviewed changes

AhyoungRyu added 14 commits June 13, 2016 10:23

Define InterpreterCompletion structure to thrift file

73e374e

Change v -> v.name & v.value in front

9e69e11

Add automatically generated thrift class

919b159

Apply new InterpreterCompletion class under zeppelin-interpreter/

6449455

Apply new InterpreterCompletion class under zeppelin-server/

dbecc51

Apply new InterpreterCompletion class under zeppelin-zengine/

c283043

Apply new InterpreterCompletion class to all interpreter class files

bd04c22

Change return type

80e295b

Fix type casting error in SparkInterpreter

24912fa

Remove console.log

43d81f6

Fix travis error

f8352c7

Remove abstract and make it return null by default

bd4f8c0

Address @jongyoul review

fdfae8f

Address @minalee feedback

9d384ec

Fix travis

311dc29

AhyoungRyu force-pushed the ZEPPELIN-982 branch from 364368e to 311dc29 Compare June 13, 2016 17:49

AhyoungRyu reviewed Jun 13, 2016
View reviewed changes

asfgit closed this in 7b00dff Jun 16, 2016

AhyoungRyu mentioned this pull request Aug 5, 2016

[ZEPPELIN-1291]Tagging meta value on interpreter completion. #1284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZEPPELIN-982] Improve interpreter completion API #984

[ZEPPELIN-982] Improve interpreter completion API #984

AhyoungRyu commented Jun 10, 2016 •

edited

Loading

jongyoul Jun 10, 2016

AhyoungRyu Jun 10, 2016

jongyoul commented Jun 10, 2016

minahlee Jun 10, 2016

AhyoungRyu commented Jun 10, 2016

jongyoul commented Jun 11, 2016

bzz commented Jun 13, 2016

bzz Jun 13, 2016

AhyoungRyu Jun 13, 2016

bzz Jun 13, 2016

AhyoungRyu commented Jun 13, 2016 •

edited

Loading

AhyoungRyu Jun 13, 2016 •

edited

Loading

Leemoonsoo commented Jun 15, 2016

[ZEPPELIN-982] Improve interpreter completion API #984

[ZEPPELIN-982] Improve interpreter completion API #984

Conversation

AhyoungRyu commented Jun 10, 2016 • edited Loading

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

jongyoul Jun 10, 2016

Choose a reason for hiding this comment

AhyoungRyu Jun 10, 2016

Choose a reason for hiding this comment

jongyoul commented Jun 10, 2016

minahlee Jun 10, 2016

Choose a reason for hiding this comment

AhyoungRyu commented Jun 10, 2016

jongyoul commented Jun 11, 2016

bzz commented Jun 13, 2016

bzz Jun 13, 2016

Choose a reason for hiding this comment

AhyoungRyu Jun 13, 2016

Choose a reason for hiding this comment

bzz Jun 13, 2016

Choose a reason for hiding this comment

AhyoungRyu commented Jun 13, 2016 • edited Loading

AhyoungRyu Jun 13, 2016 • edited Loading

Choose a reason for hiding this comment

Leemoonsoo commented Jun 15, 2016

AhyoungRyu commented Jun 10, 2016 •

edited

Loading

AhyoungRyu commented Jun 13, 2016 •

edited

Loading

AhyoungRyu Jun 13, 2016 •

edited

Loading