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 2017/08/28 13:27:51 UTC
[GitHub] zeppelin pull request #2554: [ZEPPELIN-2627] Interpreter Component Refactori...
GitHub user zjffdu opened a pull request:
https://github.com/apache/zeppelin/pull/2554
[ZEPPELIN-2627] Interpreter Component Refactoring
What is this PR for?
I didn't intended to make such large change at the beginning, but found many things are coupled together that I have to make such large change. Several suggestions for you how to review and read it.
I move the interpreter package from zeppelin-zengine to zeppelin-interpreter, this is needed for this refactoring.
The overall change is the same as I described in the design doc. I would suggest you to read the unit test first. These unit test is very readable and easy to understand what the code is doing now. InterpreterFactoryTest, InterpreterGroupTest, InterpreterSettingTest, InterpreterSettingManagerTest, RemoteInterpreterTest.
Remove the referent counting logic. Now I will kill the interpreter process as long as all the sessions in the same interpreter group is closed. (I plan to add another kind of policy for the interpreter process lifecycle control, ZEPPELIN-2197)
The RemoteFunction I introduced is for reducing code duplicates when we use RPC.
The changes in Job.java and RemoteScheduler is for fixing the race issue bug. This bug cause the flaky test we see often in ZeppelinSparkClusterTest.pySparkTest
What type of PR is it?
[Bug Fix | Refactoring]
Todos
- Task
What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-2627
How should this be tested?
Unit test is added
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-2627-2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2554.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 #2554
----
commit 13fe8d04a8a33e5df761439141a8479b09480a9a
Author: Jeff Zhang <zj...@apache.org>
Date: 2017-05-23T07:52:15Z
[ZEPPELIN-2627] Interpreter Component Refactoring
----
---
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 #2554: [ZEPPELIN-2627] Interpreter Component Refactori...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/2554
---
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 #2554: [ZEPPELIN-2627] Interpreter Component Refactoring
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2554
Thanks @Leemoonsoo for the review, I will merge it if no more comments. And will do the change in the following PR as there's still remaining work for the refactoring.
---
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 #2554: [ZEPPELIN-2627] Interpreter Component Refactoring
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the issue:
https://github.com/apache/zeppelin/pull/2554
@zjffdu Just found out an issue which is a side effect of this PR, now I cannot delete an interpreter.
By that what I mean is now say if I delete angular interpreter from the interpreter setting page, and restart zeppelin it gets recreated.
---
[GitHub] zeppelin issue #2554: [ZEPPELIN-2627] Interpreter Component Refactoring
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2554
@Leemoonsoo Do you mind if I commit it first to continue other works ?
---
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 #2554: [ZEPPELIN-2627] Interpreter Component Refactoring
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2554
I can see some test cases are removed or commented out. for example zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterTest.java and some other places.
Is it intended or work in progress?
---
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 #2554: [ZEPPELIN-2627] Interpreter Component Refactoring
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2554
Let me test this branch little more with some edge cases.
---
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 #2554: [ZEPPELIN-2627] Interpreter Component Refactoring
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the issue:
https://github.com/apache/zeppelin/pull/2554
Sure here you go [ZEPPELIN-3029](https://issues.apache.org/jira/browse/ZEPPELIN-3029)
---
[GitHub] zeppelin issue #2554: [ZEPPELIN-2627] Interpreter Component Refactoring
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2554
@Leemoonsoo I add more comments for the test case. Some testcase is not needed any 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] zeppelin issue #2554: [ZEPPELIN-2627] Interpreter Component Refactoring
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2554
hmm, I see. Because every time I will merge interpreter-setting.json into interpreter-setting.json. Mind to create a ticket ? I will fix it.
---
[GitHub] zeppelin issue #2554: [ZEPPELIN-2627] Interpreter Component Refactoring
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2554
Thanks @zjffdu for great work.
LGTM
---
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.
---