[ZEPPELIN-2627] Interpreter Component Refactoring #2554

zjffdu · 2017-08-28T13:27:48Z

What is this PR for?

I didn't intended to make such large change at the beginning, but found many things are coupled together that I have to make such large change. Several suggestions for you how to review and read it.

I move the interpreter package from zeppelin-zengine to zeppelin-interpreter, this is needed for this refactoring.
The overall change is the same as I described in the design doc. I would suggest you to read the unit test first. These unit test is very readable and easy to understand what the code is doing now. InterpreterFactoryTest, InterpreterGroupTest, InterpreterSettingTest, InterpreterSettingManagerTest, RemoteInterpreterTest.
Remove the referent counting logic. Now I will kill the interpreter process as long as all the sessions in the same interpreter group is closed. (I plan to add another kind of policy for the interpreter process lifecycle control, ZEPPELIN-2197)
The RemoteFunction I introduced is for reducing code duplicates when we use RPC.
The changes in Job.java and RemoteScheduler is for fixing the race issue bug. This bug cause the flaky test we see often in ZeppelinSparkClusterTest.pySparkTest
What type of PR is it?

[Bug Fix | Refactoring]

Todos

Task
What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-2627
How should this be tested?

Unit test is added

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 · 2017-08-28T17:19:31Z

I can see some test cases are removed or commented out. for example zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterTest.java and some other places.

Is it intended or work in progress?

zjffdu · 2017-08-30T06:37:54Z

...preter/src/test/java/org/apache/zeppelin/interpreter/InterpreterOutputChangeWatcherTest.java

    tmpDir.mkdirs();
    fileChanged = null;
-    numChanged = 0;
+    numChanged = new AtomicInteger(0);


Fix flaky test

zjffdu · 2017-08-30T06:39:59Z

...ngine/src/test/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterProcessTest.java

-        INTERPRETER_SCRIPT, "nonexists", "fakeRepo", new HashMap<String, String>(),
-        10 * 1000, null, null,"fakeName");
-    assertFalse(rip.isRunning());
-    assertEquals(0, rip.referenceCount());


These kinds of testing is not needed any more because of refactoring

zjffdu · 2017-08-30T06:43:23Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NoteInterpreterLoaderTest.java

-  private DependencyResolver depResolver;
-
-  @Before
-  public void setUp() throws Exception {


All the test here is covered by InterpreterSettingTest & InterpreterSettingManagerTest

zjffdu · 2017-08-30T10:55:34Z

@Leemoonsoo I add more comments for the test case. Some testcase is not needed any more

zjffdu · 2017-08-31T08:32:49Z

@Leemoonsoo Do you mind if I commit it first to continue other works ?

Leemoonsoo · 2017-08-31T15:38:41Z

Let me test this branch little more with some edge cases.

Leemoonsoo · 2017-08-31T18:10:36Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/scheduler/RemoteScheduler.java

+      if (job.isAborted()) {
+        job.setStatus(Status.ABORT);
+      } else if (job.getException() != null) {
+//        logger.info("Job ABORT, " + job.getId());


Shell we remove or change to logger.debug?

Leemoonsoo · 2017-08-31T18:31:05Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/InterpreterFactoryTest.java

-   * @throws Exception
-   */
  @Test
-  public void testRestartInterpreterInScopedMode() throws Exception {


@zjffdu Could you point new testcase covers this case?
InterpreterSettingManagerTest looks like have related testcase but somehow restart behavior in scoped mode seems changed. try

set an interpreter scoped mode per note

create two notes and start a paragraph in each note

Restart intepreter from a note in 'interpreter-binding' menu (not in interpreter menu)

Interpreter supposed to be restart for a note only, but it restarts interpreter for both notes.

See test here https://github.com/apache/zeppelin/pull/2554/files#diff-6e873dfc1f240d8453d17a98f2ae4d10R180
and
https://github.com/apache/zeppelin/pull/2554/files#diff-63ebbe353cb86fc2eb25d29e14e7cb18R101

Yes, those tests suppose to ensure restart behavior from note page.
However, if I try this by hands, I'm getting different behavior explained above.

Could you take a look?

@Leemoonsoo I believe this is the current behavior of zeppelin, not caused by this PR. Not sure why we would close all interpreter when it is anonymous. See
https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java#L971
If you feel OK, I will change the behavior in this PR

You're right. I have tested master branch with shiro configured and this branch without shiro.
I also not sure why close all interpreter when it is anonymous.

It's up to you change the behavior in this PR or not, but maybe handle it in separate PR for future reference?

Leemoonsoo · 2017-08-31T18:34:48Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/InterpreterFactoryTest.java

-   * @throws Exception
-   */
-  @Test
-  public void testRestartInterpreterInIsolatedMode() throws Exception {


Just like testRestartInterpreterInScopedMode() restart behavior protected by this unittest has changed.
We'll need to bring this testcase back.

https://github.com/apache/zeppelin/pull/2554/files#diff-63ebbe353cb86fc2eb25d29e14e7cb18R175

Leemoonsoo · 2017-08-31T18:38:26Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/InterpreterFactoryTest.java

-  }
-
-  @Test
-  public void testMultiUser() throws IOException, RepositoryException {


Do we have an equivalent test for this?

I think it is covered by https://github.com/apache/zeppelin/pull/2554/files#diff-63ebbe353cb86fc2eb25d29e14e7cb18R175

Leemoonsoo · 2017-08-31T18:38:53Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/InterpreterFactoryTest.java

-  }
-
-  @Test
-  public void registerCustomInterpreterRunner() throws IOException {


Do we have an equivalent test for this?

It is covered here. https://github.com/apache/zeppelin/pull/2554/files#diff-6e873dfc1f240d8453d17a98f2ae4d10R69

Leemoonsoo · 2017-08-31T18:39:19Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/InterpreterFactoryTest.java

-
-
-  @Test
-  public void getEditorSetting() throws IOException, RepositoryException, SchedulerException {


Do we have an equivalent test for this?

It is covered here https://github.com/apache/zeppelin/pull/2554/files#diff-6e873dfc1f240d8453d17a98f2ae4d10R273

Leemoonsoo · 2017-09-02T21:09:35Z

Thanks @zjffdu for great work.
LGTM

zjffdu · 2017-09-02T23:40:46Z

Thanks @Leemoonsoo for the review, I will merge it if no more comments. And will do the change in the following PR as there's still remaining work for the refactoring.

### What is this PR for? Fixed the bug mentioned in #2554 (comment) ### What type of PR is it? [Bug Fix] ### Todos * [ ] - Task ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-2998 ### How should this be tested? * Unit test is added ### 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: Jeff Zhang <zjffdu@apache.org> Closes #2626 from zjffdu/ZEPPELIN-2998 and squashes the following commits: cc11fb6 [Jeff Zhang] ZEPPELIN-2998. Fix bug in restarting interpreter in scoped mode

prabhjyotsingh · 2017-11-03T10:48:19Z

@zjffdu Just found out an issue which is a side effect of this PR, now I cannot delete an interpreter.
By that what I mean is now say if I delete angular interpreter from the interpreter setting page, and restart zeppelin it gets recreated.

zjffdu · 2017-11-03T10:57:33Z

hmm, I see. Because every time I will merge interpreter-setting.json into interpreter-setting.json. Mind to create a ticket ? I will fix it.

prabhjyotsingh · 2017-11-03T11:01:50Z

Sure here you go ZEPPELIN-3029

zjffdu force-pushed the ZEPPELIN-2627-2 branch 9 times, most recently from 6601c80 to b6065d9 Compare August 30, 2017 05:27

zjffdu commented Aug 30, 2017

View reviewed changes

zjffdu force-pushed the ZEPPELIN-2627-2 branch from b6065d9 to ad096e8 Compare August 30, 2017 06:54

zjffdu force-pushed the ZEPPELIN-2627-2 branch from ad096e8 to 4b6223d Compare August 30, 2017 10:56

Leemoonsoo reviewed Aug 31, 2017

View reviewed changes

zjffdu force-pushed the ZEPPELIN-2627-2 branch 2 times, most recently from 3a606ee to b837800 Compare September 1, 2017 11:35

zjffdu added 2 commits September 3, 2017 07:41

[ZEPPELIN-2627] Interpreter Component Refactoring

74bcb91

minor update

fa0d435

zjffdu force-pushed the ZEPPELIN-2627-2 branch from b837800 to fa0d435 Compare September 2, 2017 23:41

asfgit closed this in d6203c5 Sep 3, 2017

zjffdu mentioned this pull request Oct 17, 2017

ZEPPELIN-2998. Fix bug in restarting interpreter in scoped mode #2626

Closed

1 task



		@Test
		public void getEditorSetting() throws IOException, RepositoryException, SchedulerException {

[ZEPPELIN-2627] Interpreter Component Refactoring #2554

[ZEPPELIN-2627] Interpreter Component Refactoring #2554

Uh oh!

Conversation

zjffdu commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Leemoonsoo commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zjffdu Aug 30, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

zjffdu commented Aug 30, 2017

Uh oh!

zjffdu commented Aug 31, 2017

Uh oh!

Leemoonsoo commented Aug 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zjffdu Sep 2, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leemoonsoo commented Sep 2, 2017

Uh oh!

zjffdu commented Sep 2, 2017

Uh oh!

prabhjyotsingh commented Nov 3, 2017

Uh oh!

zjffdu commented Nov 3, 2017

Uh oh!

prabhjyotsingh commented Nov 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zjffdu commented Aug 28, 2017 •

edited

Loading

zjffdu Aug 30, 2017 •

edited

Loading

zjffdu Sep 2, 2017 •

edited

Loading