[ZEPPELIN-1012] Support Interpreter Aliases

[jongyoul]

What is this PR for?

Supporting aliases for same interpreter group and enabling running two interpreters in same group.

What type of PR is it?

[Feature]

Todos

• - Add InterpreterSettingRef
• - Add aliases feature

What is the Jira issue?

• https://issues.apache.org/jira/browse/ZEPPELIN-1012

How should this be tested?

1. Create spark2 interpreter in a interpreter tab
2. Add that interpreter in a notebook
3. run %spark and %spark2

Screenshots (if appropriate)

Questions:

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

[Leemoonsoo]

I have tested this branch and i've got some feedbacks

Interpreter-setting name and alias
I see interpreter-setting has alias field. And how about just use interpreter-setting name as an alias, for simplicity? instead of having both name and alias field.

Interpreter selection mechanism.
This PR changes current interpreter selection mechanism a lot.
For example, if i bind interpreter %md, %spark (spark, sql), %shell ... and try to use
%sql select * from table, then i get prefix not found.

I strongly recommend keep compatibility of previous interpreter selection mechanism, because it changes major user experience of Zeppelin.

My suggestion is,

1. select interpreter in previous way first. i.e. matching [group].[name] with interpreter selection text.
2. if there is no matching interpreter, start matching [alias].[name] with interpreter selection text.

while matching rule is

1. if interpreter selection text specify full [group].[name] or [alias].[name], then simply find corresponding interpreter-setting and interpreter.
2. if interpreter selection text specify just name, first assume [group] or [alias] is omitted.
3. then assume [name] is omitted.

Import and change interpreter binding

It can be a future work, but i'd like to at least have an direction how we want to handle user experience change expected this PR brings.

Before

- While Zeppelin instance have the same set of interpreters, imported notebook may run without modification
- It was possible to quickly change interpreter binding without changing every paragraph. i.e. Bind 'spark-small-cluster' interpreter setting first and then bind 'spark-large-cluster' later without changing interpreter selection text of individual paragraphs

After

- If paragraph uses alias for interpreter selection, imported notebook will not able to run unless manually change interpreter selection text for the Zeppelin instance.
- Once user want to run the same notebook with different interpreter-setting, user need to change interpreter selection text of all paragraphs

[jongyoul]

@Leemoonsoo Thanks for the review, and because this is the first working draft and I've changed a lot, some incompatibility and bug may exists. I'll fix them.

Concerning name and alias, that's also what i ask for. Keeping both of them would be redundant. And I'll check the selection mechanism. It may occurs because I've changed getInterpreter code for not using Interpreter.registeredInterpreters. I don't want to change the mechanism and should keep the current order as you mentioned. Finally, I agree to decide the direction to interpreter binding and import/export issue

[jongyoul]

@Leemoonsoo Merged name and aliases, and enabled working with 0.6.0's conf/interpreter.json. Concerning matching interpreters, I've test it with 0.6.0 binary. But there're something different from what you mentioned. If you choose %md as default and you run %sql select 1, it results in Prefix not found like what I've attached. In the code, %{name} will be adopted when default group has that name. Thus if you didn't set spark as default, you couldn't run %spark.sql only with %sql. Could you please check it again? and I've passed the CI!!

[jongyoul]

Merging if there's no more discussion

[Leemoonsoo]

Is there any special reason deprecate group and create refName instead?
In my understanding, value in refName is always exactly the same to value held by group previously.

[jongyoul]

There're two reason. The most important point is mapping interpreterSettingRef and interpreterSetting. If we keep group as the name of interpreter, InterpreterSetting.group points InterpreterSetting.name. I think it makes more difficult to understand mapping. Currently, InterpreterSetting.refName points InterpreterSettingRef.name. I thought it would be better. Second, in more minor reason, This is a bit confusion from InterpreterGroup. InterpreterSetting.group is not related interpreterGroup but because those two terms includes group in their name, it looks related. It, however, is minor issue.

[jongyoul]

And the reason why I deprecated instead of removing it is backward compatibility.

[Leemoonsoo]

If field name is used internally only, refName might help understand relation between InterpreterSetting.name and InterpreterSettingRef.name.

However, field name in InterpreterSetting change format of conf/interpreter.json and interpreter setting creation REST api.

So i'm not sure it's good idea to change file format and REST api to improve readability of internal code. Users might already have some external tools and codes that interact with conf/interpreter.json and interpreter setting creation REST api.

I think we can just add comment in the source code to clarify how InterpreterSettingRef and InterpreterSetting is mapped, without field name change.

[jongyoul]

Ok, I've got it. Actually variable names are not important and I agree that it's also good to keep that name. I'll revert those changes.

[Leemoonsoo]

@jongyoul

Regarding Prefix Not found,

Thought it was coming from this PR but found that Prefix Not found message looks like coming after ca27bf5.
Previously, in the same situation, sql interpreter not found was printed.

The difference is, previously, ZeppelinServer (InterpreterFactory, NoteReplLoader) decide which interpreter to load and print sql interpreter not found. After ca27bf5, it starts jdbc interpreter and jdbc interpreter prints it.

[Leemoonsoo]

Another behavior change i found from this PR is,

When no interpreter is binded to a notebook, (e.g. incase of conf/interpreter.json is removed)
When user open a notebook, Zeppelin automatically open interpreter binding menu and suggest default set of selections.

Previously, default set of selections were, a interpreter setting from each interpreter groups.
For example, if there are interpreter setting spark, spark2, md, sh, suggested selection will be spark, md, sh.

This PR suggest the first interpreter setting only. spark in the same situation.

[Leemoonsoo]

Is there any reason not saving it?

[jongyoul]

I've replace the function of add to add interpreterSettingRef and add new method called createNewSetting. Thus this method is called when InterpreterFactory.init() only and init method run saveToFile() at the end of method. Thus It would be redundant. But, I've missed saveToFile() at the end of createNewSetting. I'll add it into createNewSetting

[jongyoul]

I've added saveToFile() at the end of createNewSetting, pushed it.

[jongyoul]

@Leemoonsoo I've check the default binding issues and I will fix it.

[jongyoul]

@Leemoonsoo Concerning sql interpreter not found, I'll fix that issue with another one. I'll remove jdbc magic after I adopt this feature.

[jongyoul]

@Leemoonsoo I've reverted refName to group again. Review this PR finally.

[Leemoonsoo]

@jongyoul Thanks! I have tested and reviewed the code. and looks good except one behavior.

Let's say there're two interpreter settings. spark1 and spark2 (and no interpreter setting name spark).

previously '%spark' select spark1 or spark2 whichever in front of binded interpreter list.
now, '%spark' is printing Prefix not found.

So after this PR, in this specific case, notebook will not be run before user change interpreter selection text in every paragraphs.

How do you think about supporting previous interpreter selection mechanism, too? Will there be any side effect to support previous interpreter selection mechanism? Or do you think it's better support new selection mechanism only?

[jongyoul]

@Leemoonsoo I agree that this PR lower user experience in your case. It's better to support that feature for backward compatibility. BTW, I also think this makes users confused because user has no interpreter setting like spark but can run with %spark. I'll handle this issue with ZEPPELIN-1191 because it's out of scope of this issue. I merge it now and go for the next issue.

[jongyoul]

@Leemoonsoo Thanks for detail review!!