You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by zjffdu <gi...@git.apache.org> on 2016/08/16 10:25:09 UTC
[GitHub] zeppelin pull request #1333: [ZEPPELIN-1334] Environment variable defined in...
GitHub user zjffdu opened a pull request:
https://github.com/apache/zeppelin/pull/1333
[ZEPPELIN-1334] Environment variable defined in interpreter setting doesn't take effect
### What is this PR for?
I define SPAKR_HOME in interpreter setting, but it doesn't take effect. This PR is for bring back the environment variable defined in interpreter setting. The root cause is that we reset the env after creating RemoteInterpreter.
```
new RemoteInterpreter(property, noteId, className, conf.getInterpreterRemoteRunnerPath(),
interpreterPath, localRepoPath, connectTimeout, maxPoolSize,
remoteInterpreterProcessListener, appEventListener);
remoteInterpreter.setEnv(env);
```
### What type of PR is it?
[Bug Fix]
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-1334
### How should this be tested?
Tested manually. Create 2 spark interpreter setting, one for spark1 another is for spark2. And define SPARK_HOME for each interpreter. Then I can run both spark1 and spark2 in one zeppelin instance.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zjffdu/zeppelin ZEPPELIN-1334
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/1333.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1333
----
commit 3e7c1fabf364c8b6cfcb2b28fcfac0618cfb36aa
Author: Jeff Zhang <zj...@apache.org>
Date: 2016-08-16T10:16:58Z
[ZEPPELIN-1334] Environment variable defined in interpreter setting doesn't take effect
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1333
All 3 CI failing profiles: [Spark 2.0](https://s3.amazonaws.com/archive.travis-ci.org/jobs/153792236/log.txt) and 1.6 are very consistent
````
[INFO] Zeppelin: Zengine .................................. FAILURE [ 44.356 s]
Failed tests:
InterpreterFactoryTest.testRemoteRepl:118 expected:<VALUE_1> but was:<null>
Tests run: 6, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.108 sec <<< FAILURE! - in org.apache.zeppelin.interpreter.InterpreterFactoryTest
testRemoteRepl(org.apache.zeppelin.interpreter.InterpreterFactoryTest) Time elapsed: 0.012 sec <<< FAILURE!
java.lang.AssertionError: expected:<VALUE_1> but was:<null>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:743)
at org.junit.Assert.assertEquals(Assert.java:118)
at org.junit.Assert.assertEquals(Assert.java:144)
at org.apache.zeppelin.interpreter.InterpreterFactoryTest.testRemoteRepl(InterpreterFactoryTest.java:118)
```
@zjffdu could you please take a look and make sure it's not related to the changes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/1333
Merge it to master and branch-0.6 if there're no more discussions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/1333
Great work @zjffdu!
Looks good to me!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1333
Very similar test failure as we discussed above happened in un-related #1363 [here](https://s3.amazonaws.com/archive.travis-ci.org/jobs/155836675/log.txt)
```
Failed tests:
InterpreterFactoryTest.testRemoteRepl:123 expected:<VALUE_1> but was:<null>
Tests run: 6, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.146 sec <<< FAILURE! - in org.apache.zeppelin.interpreter.InterpreterFactoryTest
testRemoteRepl(org.apache.zeppelin.interpreter.InterpreterFactoryTest) Time elapsed: 0.032 sec <<< FAILURE!
java.lang.AssertionError: expected:<VALUE_1> but was:<null>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:743)
at org.junit.Assert.assertEquals(Assert.java:118)
at org.junit.Assert.assertEquals(Assert.java:144)
at org.apache.zeppelin.interpreter.InterpreterFactoryTest.testRemoteRepl(InterpreterFactoryTest.java:123)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #1333: [ZEPPELIN-1334] Environment variable defined in...
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu closed the pull request at:
https://github.com/apache/zeppelin/pull/1333
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1333
@zjffdu Looks great to me, modulo minor style issue above, thank you and :+1: for tests!
One more thing - it's a bit strange ZEPPELIN-1334 has fix-for version ONLY 0.6.2 which implies merging only to _branch-0.6_ but this PR is to _master_.
Let's merge it, as soon as those 2 issues are addressed!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/1333
Thanks @zjffdu for the fix. Let me take a look.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/1333
@bzz Sure, let me know if I need to create another PR for 0.6
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #1333: [ZEPPELIN-1334] Environment variable defined in...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/1333
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1333
@zjffdu thank you for fixing it!
After the review we better merge this to both, _master_ and _branch-0.6_
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by minahlee <gi...@git.apache.org>.
Github user minahlee commented on the issue:
https://github.com/apache/zeppelin/pull/1333
I tested branch-0.6 after reverting this PR, and I cannot reproduce the issue.
```
remoteInterpreter.setEnv(env);
```
Above line of code only existed in master branch, and use of variable `env` is causing compiling issue in branch-0.6.
If someone else can confirm that env variable works in branch-0.6 without this patch, I would like to revert this PR from branch-0.6.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the issue:
https://github.com/apache/zeppelin/pull/1333
@zjffdu so there must be some state that persists between the runs.
@Leemoonsoo do you have any idea what that state might be that affects `InterpreterFactoryTest `? There seems to be nothing like `@BeforeClass`, etc.
Shall we try re-triggering CI one more time?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/1333
@zjffdu Tested and It looks good to me.
There is an unittest for set environment variable for remote interpreter at [RemoteInterpreterTest.java](https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/test/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterTest.java#L721). However, the unit test is not testing the case this fix addresses.
It's not going to be a blocker but I think it'll be really beneficial to have a unittest that covers this fix, somewhere in [InterpreterFactoryTest.java](https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/test/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterTest.java). @zjffdu Is it going to be difficult to add the test? Let me know if you need any help!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #1333: [ZEPPELIN-1334] Environment variable defined in...
Posted by zjffdu <gi...@git.apache.org>.
GitHub user zjffdu reopened a pull request:
https://github.com/apache/zeppelin/pull/1333
[ZEPPELIN-1334] Environment variable defined in interpreter setting doesn't take effect
### What is this PR for?
I define SPAKR_HOME in interpreter setting, but it doesn't take effect. This PR is for bring back the environment variable defined in interpreter setting. The root cause is that we reset the env after creating RemoteInterpreter.
```
new RemoteInterpreter(property, noteId, className, conf.getInterpreterRemoteRunnerPath(),
interpreterPath, localRepoPath, connectTimeout, maxPoolSize,
remoteInterpreterProcessListener, appEventListener);
remoteInterpreter.setEnv(env);
```
### What type of PR is it?
[Bug Fix]
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-1334
### How should this be tested?
Tested manually. Create 2 spark interpreter setting, one for spark1 another is for spark2. And define SPARK_HOME for each interpreter. Then I can run both spark1 and spark2 in one zeppelin instance.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zjffdu/zeppelin ZEPPELIN-1334
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/1333.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1333
----
commit 3e7c1fabf364c8b6cfcb2b28fcfac0618cfb36aa
Author: Jeff Zhang <zj...@apache.org>
Date: 2016-08-16T10:16:58Z
[ZEPPELIN-1334] Environment variable defined in interpreter setting doesn't take effect
commit 50934a00ecd44a6d17ffaa5967f9ab7aadfcbb2f
Author: Jeff Zhang <zj...@apache.org>
Date: 2016-08-19T06:20:01Z
add test case
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/1333
@Leemoonsoo @bzz I just fix the unit test. The root cause is that `HeliumApplicationFactoryTest` will also register these interpreters. And all these unit tests are in the same JVM. So I change `pom.xml` to fork new jvm for each unit test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/1333
@Leemoonsoo I have added unit test, please help review. failed test seems irrelevant.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/1333
Thanks @bzz, I have fixed the code style issue. Another remaining thing is that the unit test I added is failed when running it using maven with other unit test. But it succeeded when I ran it individually. this is very weird.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/1333
@Leemoonsoo Please help to review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/1333
PR rebased.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---