Repository: zeppelin Updated Branches: refs/heads/master 5595bfb9d -> 3389e8cfc
[ZEPPELIN-1666] DON'T share mutable deps, options between interpreters in each group (bug) ### What is this PR for? Every interpreter shares their `List<Dependency>` and `InterpreterOption` object with other interpreters in the same group since these objects are mutable and just returned from `InterpreterSettingRef` in InterpreterFactory. I attached GIF ### What type of PR is it? [Bug Fix] ### Todos Nothing ### What is the Jira issue? [ZEPPELIN-1666](https://issues.apache.org/jira/browse/ZEPPELIN-1666) ### How should this be tested? I included unit test for it in `InterpreterRestApiTest.testCreatedInterpreterDependencies`. You can reproduce and debug this by checking out commit 3acde56 and run the unit test. If you try to debug, you can see the length of `List<Dependency>` in `md` `InterpreterSettingRef` is increased whenever creating a new interpreter setting. But it shouldn't be. ### Screenshots (if appropriate)  ### Questions: * Does the licenses files need update? - NO * Is there breaking changes for older versions? - NO * Does this needs documentation? - NO Author: 1ambda <1am...@gmail.com> Closes #1638 from 1ambda/fix/do-not-share-deps-in-same-group and squashes the following commits: e95297c [1ambda] fix: Styling issue in zengine 7825108 [1ambda] fix: mutable object problems in InterpreterSetting 5460241 [1ambda] fix: Return immutable objects 3acde56 [1ambda] test: Add failing test Project: http://git-wip-us.apache.org/repos/asf/zeppelin/repo Commit: http://git-wip-us.apache.org/repos/asf/zeppelin/commit/3389e8cf Tree: http://git-wip-us.apache.org/repos/asf/zeppelin/tree/3389e8cf Diff: http://git-wip-us.apache.org/repos/asf/zeppelin/diff/3389e8cf Branch: refs/heads/master Commit: 3389e8cfcbb8618102cd4894339e1a1828bbc837 Parents: 5595bfb Author: 1ambda <1am...@gmail.com> Authored: Thu Nov 24 15:12:23 2016 +0900 Committer: Alexander Bezzubov <b...@apache.org> Committed: Thu Nov 24 18:33:15 2016 +0900 ---------------------------------------------------------------------- .../zeppelin/interpreter/InterpreterOption.java | 16 +++++ .../zeppelin/rest/InterpreterRestApiTest.java | 68 ++++++++++++++++++++ .../interpreter/InterpreterFactory.java | 15 +++-- 3 files changed, 95 insertions(+), 4 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/zeppelin/blob/3389e8cf/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOption.java ---------------------------------------------------------------------- diff --git a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOption.java b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOption.java index 3e789ba..1e9c1cc 100644 --- a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOption.java +++ b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOption.java @@ -17,6 +17,7 @@ package org.apache.zeppelin.interpreter; +import java.util.ArrayList; import java.util.List; /** @@ -96,6 +97,21 @@ public class InterpreterOption { this.perNote = perNote; } + public static InterpreterOption fromInterpreterOption(InterpreterOption other) { + InterpreterOption option = new InterpreterOption(); + option.remote = other.remote; + option.host = other.host; + option.port = other.port; + option.perNote = other.perNote; + option.perUser = other.perUser; + option.isExistingProcess = other.isExistingProcess; + option.setPermission = other.setPermission; + option.users = (null == other.users) ? + new ArrayList<String>() : new ArrayList<>(other.users); + + return option; + } + public boolean isRemote() { return remote; } http://git-wip-us.apache.org/repos/asf/zeppelin/blob/3389e8cf/zeppelin-server/src/test/java/org/apache/zeppelin/rest/InterpreterRestApiTest.java ---------------------------------------------------------------------- diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/InterpreterRestApiTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/InterpreterRestApiTest.java index 8db6650..518ce6a 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/InterpreterRestApiTest.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/InterpreterRestApiTest.java @@ -18,11 +18,14 @@ package org.apache.zeppelin.rest; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import java.util.Map; import com.google.gson.JsonArray; import com.google.gson.JsonElement; import com.google.gson.JsonObject; +import com.google.gson.reflect.TypeToken; import org.apache.commons.httpclient.methods.DeleteMethod; import org.apache.commons.httpclient.methods.GetMethod; import org.apache.commons.httpclient.methods.PostMethod; @@ -44,6 +47,7 @@ import org.junit.runners.MethodSorters; import com.google.gson.Gson; import static org.junit.Assert.*; +import static org.hamcrest.MatcherAssert.assertThat; /** * Zeppelin interpreter rest api tests @@ -147,6 +151,70 @@ public class InterpreterRestApiTest extends AbstractTestRestApi { } @Test + public void testCreatedInterpreterDependencies() throws IOException { + // when: Create 2 interpreter settings `md1` and `md2` which have different dep. + + String md1Name = "md1"; + String md2Name = "md2"; + + String md1Dep = "org.apache.drill.exec:drill-jdbc:jar:1.7.0"; + String md2Dep = "org.apache.drill.exec:drill-jdbc:jar:1.6.0"; + + String reqBody1 = "{\"name\":\"" + md1Name + "\",\"group\":\"md\",\"properties\":{\"propname\":\"propvalue\"}," + + "\"interpreterGroup\":[{\"class\":\"org.apache.zeppelin.markdown.Markdown\",\"name\":\"md\"}]," + + "\"dependencies\":[ {\n" + + " \"groupArtifactVersion\": \"" + md1Dep + "\",\n" + + " \"exclusions\":[]\n" + + " }]," + + "\"option\": { \"remote\": true, \"session\": false }}"; + PostMethod post = httpPost("/interpreter/setting", reqBody1); + assertThat("test create method:", post, isCreated()); + post.releaseConnection(); + + String reqBody2 = "{\"name\":\"" + md2Name + "\",\"group\":\"md\",\"properties\":{\"propname\":\"propvalue\"}," + + "\"interpreterGroup\":[{\"class\":\"org.apache.zeppelin.markdown.Markdown\",\"name\":\"md\"}]," + + "\"dependencies\":[ {\n" + + " \"groupArtifactVersion\": \"" + md2Dep + "\",\n" + + " \"exclusions\":[]\n" + + " }]," + + "\"option\": { \"remote\": true, \"session\": false }}"; + post = httpPost("/interpreter/setting", reqBody2); + assertThat("test create method:", post, isCreated()); + post.releaseConnection(); + + // 1. Call settings API + GetMethod get = httpGet("/interpreter/setting"); + String rawResponse = get.getResponseBodyAsString(); + get.releaseConnection(); + + // 2. Parsing to List<InterpreterSettings> + JsonObject responseJson = gson.fromJson(rawResponse, JsonElement.class).getAsJsonObject(); + JsonArray bodyArr = responseJson.getAsJsonArray("body"); + List<InterpreterSetting> settings = new Gson().fromJson(bodyArr, + new TypeToken<ArrayList<InterpreterSetting>>() { + }.getType()); + + // 3. Filter interpreters out we have just created + InterpreterSetting md1 = null; + InterpreterSetting md2 = null; + for (InterpreterSetting setting : settings) { + if (md1Name.equals(setting.getName())) { + md1 = setting; + } else if (md2Name.equals(setting.getName())) { + md2 = setting; + } + } + + // then: should get created interpreters which have different dependencies + + // 4. Validate each md interpreter has its own dependencies + assertEquals(1, md1.getDependencies().size()); + assertEquals(1, md2.getDependencies().size()); + assertEquals(md1Dep, md1.getDependencies().get(0).getGroupArtifactVersion()); + assertEquals(md2Dep, md2.getDependencies().get(0).getGroupArtifactVersion()); + } + + @Test public void testSettingsCreateWithEmptyJson() throws IOException { // Call Create Setting REST API PostMethod post = httpPost("/interpreter/setting/", ""); http://git-wip-us.apache.org/repos/asf/zeppelin/blob/3389e8cf/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterFactory.java ---------------------------------------------------------------------- diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterFactory.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterFactory.java index 4564e3a..610561c 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterFactory.java +++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterFactory.java @@ -285,10 +285,17 @@ public class InterpreterFactory implements InterpreterGroupFactory { } private InterpreterSetting createFromInterpreterSettingRef(InterpreterSetting o) { - InterpreterSetting setting = - new InterpreterSetting(o.getName(), o.getName(), o.getInterpreterInfos(), - convertInterpreterProperties((Map<String, InterpreterProperty>) o.getProperties()), - o.getDependencies(), o.getOption(), o.getPath()); + // should return immutable objects + List<InterpreterInfo> infos = (null == o.getInterpreterInfos()) ? + new ArrayList<InterpreterInfo>() : new ArrayList<>(o.getInterpreterInfos()); + List<Dependency> deps = (null == o.getDependencies()) ? + new ArrayList<Dependency>() : new ArrayList<>(o.getDependencies()); + Properties props = + convertInterpreterProperties((Map<String, InterpreterProperty>) o.getProperties()); + InterpreterOption option = InterpreterOption.fromInterpreterOption(o.getOption()); + + InterpreterSetting setting = new InterpreterSetting(o.getName(), o.getName(), + infos, props, deps, option, o.getPath()); setting.setInterpreterGroupFactory(this); return setting; }
