ZEPPELIN-1254 Make get and save Interpreter bindings calls via websocket #1247

r-kamath · 2016-07-29T13:04:48Z

What is this PR for?

This PR uses websocket to get and save interpreter bindings instead of rest api

What type of PR is it?

Improvement

Todos

- Task

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1254

How should this be tested?

Go to interpreter binding settings on notebook page and enable/disable interpreter or reorder and save. Simultaneously verify the network calls on browser's console.

Screenshots (if appropriate)

n/a

Questions:

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

prabhjyotsingh · 2016-07-29T14:26:39Z

zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java

+    }
+
+    List<InterpreterSetting> availableSettings = notebook().getInterpreterFactory().get();
+    for (InterpreterSetting setting : availableSettings) {


I see duplicate effort in this, can you move this into a common function which both this and https://github.com/apache/zeppelin/blob/master/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java#L197-L211 can use ?

jongyoul · 2016-07-31T07:06:20Z

This PR looks like copy and paste from NotebookRestAapi. If you think websocket is a better way to do it, why don't you removing rest api? I don't think it's good to duplicate only. What's the benefit to change rest api to websocket?

jongyoul · 2016-07-31T07:07:35Z

And please add test case if you are going further.

r-kamath · 2016-07-31T08:31:46Z

@jongyoul Yes. this is copied from rest api. We should probably keep both options and use websocket on zeppelin UI. IMHO we all communications on the ui should happen via websockets and avoid multiple calls to server. I'll be cleaning up the duplication in my next commit. @prabhjyotsingh also had similar comment on code duplication.

jongyoul · 2016-07-31T10:08:49Z

zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java

 import org.apache.zeppelin.interpreter.InterpreterGroup;
 import org.apache.zeppelin.interpreter.remote.RemoteAngularObjectRegistry;
 import org.apache.zeppelin.interpreter.thrift.InterpreterCompletion;
+import org.apache.zeppelin.rest.message.InterpreterSettingListForNoteBind;


I think you'd better move this class into proper one. It's used in binding method only.

jongyoul · 2016-07-31T10:11:24Z

Thanks for the explaining it, but I think you'd better handle those changes into single PR, which is more meaningful. How do you think of it?

r-kamath · 2016-07-31T10:18:51Z

@jongyoul I'm planning to make those changes in this PR

jongyoul · 2016-07-31T10:41:57Z

That sounds great

bzz · 2016-08-01T01:15:18Z

@r-kamath if you are planning to add more changes in this PR, do you think it will make sense to:

update TODO list in PR description with action items that you plan
add WIP prefix (work in progress) to the PR title, and remove it when everything is done

In my experience, this will help to communicate your intent with reviewers, save time for them and overall speed up the process of merging these changes.

prabhjyotsingh · 2016-08-02T14:25:48Z

zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java

+    String noteID = (String) fromMessage.data.get("noteID");
+    List<InterpreterSettingsList> settingList =
+        InterpreterBindingUtils.getInterpreterBindings(notebook(), noteID);
+    broadcastInterpreterBindings(noteID, settingList);


Instead of broadcast can you just conn.send(serializeMessage)

fixed 55d95ec

prabhjyotsingh · 2016-08-02T16:08:56Z

LGTM!

jongyoul · 2016-08-03T08:53:50Z

zeppelin-server/src/main/java/org/apache/zeppelin/types/InterpreterSettingsList.java

-  public InterpreterSettingListForNoteBind(String id, String name,
-      List<InterpreterInfo> interpreters, boolean selected) {
+  public InterpreterSettingsList(String id, String name,
+                                 List<InterpreterInfo> interpreters, boolean selected) {


Vertial align is not recommended. Could you please revert this into original one?

will update

jongyoul · 2016-08-03T11:12:17Z

@r-kamath Don't you remove the rest api in this PR? If you do that, you don't have to make getInterpreterBinding as a static because it's only used in notebook server. I thought you would removed interpreter binding rest api entry points and changed all of them into websocket.

r-kamath · 2016-08-03T12:23:58Z

@jongyoul I think we have users for rest api. The change here is to make ui work via websocket while keeping the reset apis intact.

jongyoul · 2016-08-03T13:46:47Z

@r-kamath But I thought you would removed rest api by your last comments. Do you think we should conserve both of them?

r-kamath · 2016-08-03T13:58:25Z

@jongyoul Yes. we should keep both

jongyoul · 2016-08-03T13:59:11Z

@r-kamath Actually, I don't know well how much efficient we change rest api to websocket because I'm not at front-end. I just wonder if we should keep two different entry point with exact same function.

jongyoul · 2016-08-03T14:00:44Z

@r-kamath Of course, I agree that it's good to support some apis with websocket, basically.

jongyoul · 2016-08-03T14:02:20Z

But I think this is not proper scope of this PR. We need to open discussion on it. Your PR looks good to me.

prabhjyotsingh · 2016-08-05T14:28:37Z

Merging this if no more discussion.

### What is this PR for? This PR uses websocket to get and save interpreter bindings instead of rest api ### What type of PR is it? Improvement ### Todos * [ ] - Task ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-1254 ### How should this be tested? Go to interpreter binding settings on notebook page and enable/disable interpreter or reorder and save. Simultaneously verify the network calls on browser's console. ### Screenshots (if appropriate) n/a ### Questions: * Does the licenses files need update? n/a * Is there breaking changes for older versions? n/a * Does this needs documentation? n/a Author: Renjith Kamath <renjith.kamath@gmail.com> Closes apache#1247 from r-kamath/ZEPPELIN-1254 and squashes the following commits: 6ac1748 [Renjith Kamath] ZEPPELIN-1254 revert indentation 55d95ec [Renjith Kamath] ZEPPELIN-1254 review fix ab0bced [Renjith Kamath] ZEPPELIN-1254 boradcast after updating the interpreter bindings 299bb51 [Renjith Kamath] ZEPPELIN-1254 review fix 97bfd7d [Renjith Kamath] ZEPPELIN-1254 Make get and save Interpreter bindings calls via websocket

ZEPPELIN-1254 Make get and save Interpreter bindings calls via websocket

97bfd7d

prabhjyotsingh reviewed Jul 29, 2016
View reviewed changes

jongyoul reviewed Jul 31, 2016
View reviewed changes

ZEPPELIN-1254 review fix

299bb51

r-kamath changed the title ~~ZEPPELIN-1254 Make get and save Interpreter bindings calls via websocket~~ [WIP] ZEPPELIN-1254 Make get and save Interpreter bindings calls via websocket Aug 2, 2016

ZEPPELIN-1254 boradcast after updating the interpreter bindings

ab0bced

r-kamath changed the title ~~[WIP] ZEPPELIN-1254 Make get and save Interpreter bindings calls via websocket~~ ZEPPELIN-1254 Make get and save Interpreter bindings calls via websocket Aug 2, 2016

prabhjyotsingh reviewed Aug 2, 2016
View reviewed changes

ZEPPELIN-1254 review fix

55d95ec

jongyoul reviewed Aug 3, 2016
View reviewed changes

ZEPPELIN-1254 revert indentation

6ac1748

asfgit closed this in f71d09c Aug 8, 2016

ZEPPELIN-1254 Make get and save Interpreter bindings calls via websocket #1247

ZEPPELIN-1254 Make get and save Interpreter bindings calls via websocket #1247

Uh oh!

Conversation

r-kamath commented Jul 29, 2016

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:

Uh oh!

prabhjyotsingh Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

jongyoul commented Jul 31, 2016

Uh oh!

jongyoul commented Jul 31, 2016

Uh oh!

r-kamath commented Jul 31, 2016

Uh oh!

jongyoul Jul 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jongyoul commented Jul 31, 2016

Uh oh!

r-kamath commented Jul 31, 2016

Uh oh!

jongyoul commented Jul 31, 2016

Uh oh!

bzz commented Aug 1, 2016

Uh oh!

prabhjyotsingh Aug 2, 2016

Choose a reason for hiding this comment

Uh oh!

r-kamath Aug 2, 2016

Choose a reason for hiding this comment

Uh oh!

prabhjyotsingh commented Aug 2, 2016

Uh oh!

jongyoul Aug 3, 2016

Choose a reason for hiding this comment

Uh oh!

r-kamath Aug 3, 2016

Choose a reason for hiding this comment

Uh oh!

jongyoul commented Aug 3, 2016

Uh oh!

r-kamath commented Aug 3, 2016

Uh oh!

jongyoul commented Aug 3, 2016

Uh oh!

r-kamath commented Aug 3, 2016

Uh oh!

jongyoul commented Aug 3, 2016

Uh oh!

jongyoul commented Aug 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jongyoul commented Aug 3, 2016

Uh oh!

prabhjyotsingh commented Aug 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jongyoul Jul 31, 2016 •

edited

Loading

jongyoul commented Aug 3, 2016 •

edited

Loading