[ZEPPELIN-1972] Preserve context classloader

[DrIgor]

What is this PR for?

IMain.interpret changes current thread's context classloader. It may cause different issues and definitely is the reason of ZEPPELIN-1738 test failures.

It's a known scala bug. See https://issues.scala-lang.org/browse/SI-9587

As a workaround we need to save and restore context classloader manually

What type of PR is it?

Bug Fix

What is the Jira issue?

ZEPPELIN-1972

How should this be tested?

Run ignite interpreter test and ignite sql interpreter test in the same thread

mvn test -pl ignite -am -Pscala-2.11 -Dtest=org.apache.zeppelin.ignite.IgniteInterpreterTest#testInterpret,org.apache.zeppelin.ignite.IgniteSqlInterpreterTest#testSql -DfailIfNoTests=false

Questions:

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

[jongyoul]

LGMT. I have a question. Is there any related issue to SparkInterpreter? Can you take a look into it?

[DrIgor]

@jongyoul I checked SparkInterpreter, it has the same issue with context class loader

[Leemoonsoo]

Thanks @DrIgor for fix and explanation of the issue!

When RemoteInterpreterServer creates Interpreter, it wraps interpreter instance with LazyOpenInterpreter. If we wrap interpreter instance with ClassLoaderInterpreter as well, like zeppelin do when creating interpreter locally, then every interpreter will benefit by ClassloaderInterpreter. ClassloaderInterpreter restores classloader after methods call.

@DrIgor@jongyoul Shell we try make RemoteInterpreterServer wrap interpreter with ClassloaderInterpreter? what do you think?

[jongyoul]

@Leemoonsoo I think to wrap it with ClassloaderInterpreter would be a good idea. @DrIgor Can you try it on?

[DrIgor]

@Leemoonsoo@jongyoul thank you! I didn't know about ClassLoaderInterpreter

Unfortunately, our tests don't use InterpreterFactory and will use unwrapped Interpreter instance. So It won't fix flaky test.

Besides, code inside interpreter itself will use incorrect classloader. Currently it isn't an issue but someday...

Maybe It's better to wrap IMain or create "scala utils" to share common code between several scala-speaking interpreters?

[jongyoul]

It actually wrap and conserve classloader for interpreter. I think you just change this line https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java#L192 to use ClassLoaderInterpreter. Can you try it?

[jongyoul]

Concerning ClassloaderInterpreter, you can find it https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/ClassloaderInterpreter.java Can you check this class if it can fix this issue?

[DrIgor]

Yes, I tried ClassloaderInterpreter but it didn't fix original issue.

https://github.com/apache/zeppelin/blob/master/ignite/src/test/java/org/apache/zeppelin/ignite/IgniteInterpreterTest.java#L69
Here test creates unwrapped interpreter instance.

So we need wrap every IgniteInterpreter usage. Besides, wrapper restores classloader after interpreter call and interpreter itself use broken classloader.

[jongyoul]

Got it. How about wrapping it like new ClassloaderInterpreter(new IgniteInterpreter(props))? It's the same logic that RemoteInterpreter want to work. You should change this line https://github.com/apache/zeppelin/blob/master/ignite/src/test/java/org/apache/zeppelin/ignite/IgniteInterpreterTest.java#L45 to Interpreter

[jongyoul]

Is there any update on it?

[DrIgor]

I tried to wrap with ClassloaderInterpreter and it works

But I don't like this solution. We force every user of IgniteInterpreter to wrap it with ClassloaderInterpreter. It's very easy to broke this test again. For example, to add new test and forget about wrapping.

It looks like dirty fix for me. IgniteInterpreter is still broker and we try to reduce negative effects by wrapper. I think it is acceptable for third party libraries when we can't change sources. But if we can fix original code we should do it

If you think that wrapper is more appropriate I will update PR

[jongyoul]

@DrIgor I agree with you. It's very easily broken. But on the other hands, I will make a new PR for changing ClassloaderInterpreter because another interpreter will fails with same issue if it doesn't implement like you.

LGTM, will merge it.