[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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: but was: 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: but was: 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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: but was: 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: but was: 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #1333: [ZEPPELIN-1334] Environment variable defined in interp...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
