[ZEPPELIN-535] "Scheduler already terminated" occurs when RemoteInterpreter.close() doesn't succeed

[Leemoonsoo]

What is this PR for?

Fix the exception "Scheduler already terminated" when remove interpreter close() fails

What type of PR is it?

Bug Fix

Is there a relevant Jira issue?

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

How should this be tested?

Modify any interpreter to throw exception on close() call.
And try to use it after restart the interpreter.

Screenshots (if appropriate)

Questions:

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

[HeartSaVioR]

Great for reducing complexity, modified code looks good to me.
Btw, CI test is failing,

Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 6.548 sec <<< FAILURE! - in org.apache.zeppelin.rest.ZeppelinSparkClusterTest
org.apache.zeppelin.rest.ZeppelinSparkClusterTest  Time elapsed: 6.547 sec  <<< ERROR!
java.lang.NullPointerException: null
    at org.apache.zeppelin.rest.AbstractTestRestApi.startUp(AbstractTestRestApi.java:127)
    at org.apache.zeppelin.rest.ZeppelinSparkClusterTest.init(ZeppelinSparkClusterTest.java:48)

Related code block is here,

// assume first one is spark
        InterpreterSetting sparkIntpSetting = null;
        for(InterpreterSetting intpSetting : ZeppelinServer.notebook.getInterpreterFactory().get()) {
          if (intpSetting.getGroup().equals("spark")) {
            sparkIntpSetting = intpSetting;
          }
        }

Does this occur first time? Maybe we can't rely on sequence of interpreter setting now?

[Leemoonsoo]

@HeartSaVioR now ci is passing

This PR make sure RemoteInterpreter.close() call InterpreterProcess.dereference().
Also simplify code by let InterpreterGroup keep reference to InterpreterProcess instead of static Map<String, RemoteInterpreterProcess> interpreterGroupReference.

[HeartSaVioR]

@Leemoonsoo
Yeah, actually interpreterGroupReference makes me hard to read (and understand) Zeppelin's interpreter source code. Great for removing it.

[Leemoonsoo]

@HeartSaVioR Thanks for raising issue and taking review.
Merge if there're no more discussions.

[HeartSaVioR]

@Leemoonsoo
Thanks for the quick fix!
Btw, skimming review process from CONTRIBUTING.md, it may require one additional committer to be reviewed (have a right to bind voting). Do I understand right?

[minahlee]

@HeartSaVioR It's not mandatory to have review from PPMC/committer since we follow lazy consensus rule but always good to be reviewed by more people including non-committer.
Check 5th bullet from The Review Process for the detail.

[Leemoonsoo]

According to CONTRIBUTING.md, a committer review is required, not one additional committer. So technically, nothing stops committer push self reviewed code. However, in Zeppelin project, committers used to initiate lazy consensus ("merge if there 're no more discussions") instead of pushing self reviewed code directly, when there're no review from other committers.

[HeartSaVioR]

@minahlee @Leemoonsoo
Thanks for letting me know. Zeppelin's review process seems a bit uncommon to me so I was confused. It's cleared, and I can see the benefit.

[Leemoonsoo]

@HeartSaVioR Thanks for asking. Please remember anyone can create a pullrequest to change CONTRIBUTING.md to improve review process, and more.