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/06/21 08:14:11 UTC
[GitHub] zeppelin pull request #2422: [WIP] [ZEPPELIN-2627] Interpreter refactor
GitHub user zjffdu opened a pull request:
https://github.com/apache/zeppelin/pull/2422
[WIP] [ZEPPELIN-2627] Interpreter refactor
### What is this PR for?
A few sentences describing the overall goals of the pull request's commits.
First time? Check out the contributing guide - https://zeppelin.apache.org/contribution/contributions.html
### What type of PR is it?
[Bug Fix | Improvement | Feature | Documentation | Hot Fix | Refactoring]
### Todos
* [ ] - Task
### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN/
* Put link here, and add [ZEPPELIN-*Jira number*] in PR title, eg. [ZEPPELIN-533]
### How should this be tested?
Outline the steps to test the PR here.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update?
* Is there breaking changes for older versions?
* Does this needs documentation?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zjffdu/zeppelin interpreter_refactor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2422.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 #2422
----
commit 8ee3f45dffa7feff4b95f0ac0cdf6ac3fc69f61b
Author: Jeff Zhang <zj...@apache.org>
Date: 2017-05-23T07:52:15Z
save
commit 2ca2188cd157ec509b5b982ffeaaae2d3f954d07
Author: Jeff Zhang <zj...@apache.org>
Date: 2017-06-18T05:52:26Z
fix Job race
----
---
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 #2422: [ZEPPELIN-2627] Interpreter refactor
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/2422
---
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 #2422: [WIP] [ZEPPELIN-2627] Interpreter refactor
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2422
Thanks @prabhjyotsingh for testing this PR.
---
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 #2422: [WIP] [ZEPPELIN-2627] Interpreter refactor
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2422
@Leemoonsoo @jongyoul @prabhjyotsingh @felixcheung Could you help review that ?
Here's some suggestion for you how to review it since it is a very large PR
* 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 reference 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
---
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 #2422: [WIP] [ZEPPELIN-2627] Interpreter refactor
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/2422
Wow ~ It changes pretty big including some code styles. Before reviewing it, How about reverting code style? It's not related to feature itself, then we can fix the style with minor pr. And can you describe what is the purpose of this PR? Which logic do you want to refactor? this kind of description will be help to review it. I glanced at it few minutes, and it seems to relocate some classes and introduce remoteFunction concept. Can you explain them 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 pull request #2422: [WIP] [ZEPPELIN-2627] Interpreter refactor
Posted by zjffdu <gi...@git.apache.org>.
GitHub user zjffdu reopened a pull request:
https://github.com/apache/zeppelin/pull/2422
[WIP] [ZEPPELIN-2627] Interpreter refactor
### 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 interpreter_refactor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2422.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 #2422
----
commit ca60c5f3b33420ae48f2128daa4f4492301f083b
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 issue #2422: [WIP] [ZEPPELIN-2627] Interpreter refactor
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2422
Thanks @herval for the review. If you have time, you can also check the unit test which is much more readable and will easy for you to get what I done in this PR. (InterpreterFactoryTest, InterpreterGroupTest, InterpreterSettingTest, InterpreterSettingManagerTest, RemoteInterpreterTest)
Thanks again for your kind 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 #2422: [WIP] [ZEPPELIN-2627] Interpreter refactor
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the issue:
https://github.com/apache/zeppelin/pull/2422
Tried this on my local cluster, tested it with simultaneously with multiple users. Looks good. Great improvement.
---
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 #2422: [ZEPPELIN-2627] Interpreter refactor
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2422
@Leemoonsoo @jongyoul I will commit if no more comments so that I can continue the other work.
---
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 #2422: [WIP] [ZEPPELIN-2627] Interpreter refactor
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu closed the pull request at:
https://github.com/apache/zeppelin/pull/2422
---
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 #2422: [ZEPPELIN-2627] Interpreter refactor
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2422
Sorry, I found it conflicts with other PRs, will revert it now.
---
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 #2422: [WIP] [ZEPPELIN-2627] Interpreter refactor
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2422
@jongyoul Thanks for the quick review. 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 didn't intended to make the code style change, this is due to I move some files in intellij and intellij automatically fix the code style issue. You can just ignore these code style changes
* 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`,
* 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
---
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.
---