You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by RPCMoritz <gi...@git.apache.org> on 2015/10/16 16:53:03 UTC
[GitHub] incubator-zeppelin pull request: ZEPPELIN-346 Fix NotebookTest not...
GitHub user RPCMoritz opened a pull request:
https://github.com/apache/incubator-zeppelin/pull/347
ZEPPELIN-346 Fix NotebookTest not passing non-default group to NoteIn…
…terpreterLoader
This implements the actually required logic for this test to pass and should mostly showcase that the NoteIntepreterLoader logic is flawed/broken. Nonetheless this would fix the test in the short term and be a step towards actually documenting the weird behaviour.
I will try to amend this PR (or create a separate one) to actually approach the underlying issue
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/RPCMoritz/incubator-zeppelin patch-2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-zeppelin/pull/347.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 #347
----
commit 0badf282fb02d4229ce3f476ffb157eb7dcc1c69
Author: Rick Moritz <ra...@gmail.com>
Date: 2015-10-16T14:52:38Z
ZEPPELIN-346 Fix NotebookTest not passing non-default group to NoteInterpreterLoader
This implements the actually required logic for this test to pass and should mostly showcase that the NoteIntepreterLoader logic is flawed/broken. Nonetheless this would fix the test in the short term and be a step towards actually documenting the weird behaviour.
I will try to amend this PR (or create a separate one) to actually approach the underlying issue
----
---
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] incubator-zeppelin pull request: ZEPPELIN-346 Fix NotebookTest not...
Posted by RPCMoritz <gi...@git.apache.org>.
Github user RPCMoritz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/347#issuecomment-168988570
I will re-test on my systems ASAP.
---
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] incubator-zeppelin pull request: ZEPPELIN-346 Fix NotebookTest not...
Posted by RPCMoritz <gi...@git.apache.org>.
Github user RPCMoritz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/347#issuecomment-152497944
@Leemoonsoo if you plan to release a new version, then you should probably merge this as-is.
A fix of the underlying logic is going to be more involved and won't happen too soon - I'll still attempt it though.
---
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] incubator-zeppelin pull request: ZEPPELIN-346 Fix NotebookTest not...
Posted by RPCMoritz <gi...@git.apache.org>.
Github user RPCMoritz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/347#issuecomment-152624067
On any machine I attempted to build, I got the test failure (interpreter not found). I'll try again with the current master on yet another machine to see if I can still reproduce.
---
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] incubator-zeppelin pull request: ZEPPELIN-346 Fix NotebookTest not...
Posted by RPCMoritz <gi...@git.apache.org>.
Github user RPCMoritz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/347#issuecomment-170016876
testSelectingReplImplementation(org.apache.zeppelin.notebook.NotebookTest) Time elapsed: 0.032 sec <<< ERROR!
org.apache.zeppelin.interpreter.InterpreterException: mock2 interpreter not found
at org.apache.zeppelin.notebook.NoteInterpreterLoader.get(NoteInterpreterLoader.java:148)
at org.apache.zeppelin.notebook.Note.run(Note.java:365)
at org.apache.zeppelin.notebook.NotebookTest.testSelectingReplImplementation(NotebookTest.java:119)
with
Apache Maven 3.2.5 (12a6b3acb947671f09b81f49094c53f426d8cea1; 2014-12-14T18:29:23+01:00)
Maven home: /usr/share/maven-bin-3.2
Java version: 1.7.0_80, vendor: Oracle Corporation
Java home: /opt/oracle-jdk-bin-1.7.0.80/jre
Default locale: en_US, platform encoding: ISO-8859-15
OS name: "linux", version: "3.16.5-gentoo", arch: "amd64", family: "unix"
So, still broken.
---
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] incubator-zeppelin pull request: ZEPPELIN-346 Fix NotebookTest not...
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/347#issuecomment-168948827
@RPCMoritz @FRosner does the issue still exist on the latest master?
---
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] incubator-zeppelin pull request: ZEPPELIN-346 Fix NotebookTest not...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/347#issuecomment-152674503
@Leemoonsoo the test also failed on my machine at the time the patch was proposed.
I still think, however, that passing two parameters as one by splitting a String is a very bad idea. See https://issues.apache.org/jira/browse/ZEPPELIN-346?focusedCommentId=14960634&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14960634
---
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] incubator-zeppelin pull request: ZEPPELIN-346 Fix NotebookTest not...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/347#issuecomment-152610750
@RPCMoritz Thanks for making the patch and interest to Zeppelin.
Selecting interpreter using %[group].[name] notation introduced by https://issues.apache.org/jira/browse/ZEPPELIN-74. Main reason the logic working in this way is to keep compatibility with previous behavior, ie. to keep selecting interpreter simple.
Guessing interpreter from partial string (%[group] or %[name]) is intended behavior.
Only confusion zeppelin does not handle right now is when there're '.' (dot) in [group] or [name] string.
So, I'd like to understand more about why do you think there're bad test and ambiguous logic, and what are you trying to fix in the end. Could you explain more?
---
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] incubator-zeppelin pull request: ZEPPELIN-346 Fix NotebookTest not...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/347#issuecomment-152621780
Could you help me to reproduce the test error? Because of the test also passes in my machines, both using mvn test command and run inside of IDE.
---
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] incubator-zeppelin pull request: ZEPPELIN-346 Fix NotebookTest not...
Posted by RPCMoritz <gi...@git.apache.org>.
Github user RPCMoritz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/347#issuecomment-152616329
I believe the test currently only passes CI, because of the broken implementation of using System properties. Without setting group.name explicetely, the guessing code does not correctly guess the second interpreter by name only, because the structure used does not match how it's written into system properties (in this test). The actual issue may be in the module writing system properties, but I assert that using System properties in this way is not ideal, and using API calls or a dedicated configuration object instead is more useful (but still not generally independent across multiple notebooks and therefore influenced by tests being run in parallel)
So the current fix to the test makes tests pass in linear execution.
My proposed fix of the actual logic is to either
a) fix writing/parsing System properties
or
b) move to an object oriented design and remove the use of System properties for internal variables, if possible.
I looked at this issue together with @FRosner , who gave his commentary in the JIRA issue, which may be helpful in making my point.
---
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.
---