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/09/19 06:54:36 UTC
[GitHub] zeppelin pull request #2592: ZEPPELIN-2685. Improvement on Interpreter class
GitHub user zjffdu opened a pull request:
https://github.com/apache/zeppelin/pull/2592
ZEPPELIN-2685. Improvement on Interpreter class
### What is this PR for?
Main changes on Interpreter
* Add throw InterpreterException for the abstract methods of `Interpreter`, this would enforce the implementation to throw `InterpreterException`.
* field name refactoring.
** `property` -> `properties`
** `getProperty()` --> `getProperties()`
** `setProperty()` --> `setProperties`
* Introduce launcher layer for interpreter launching.
** abstract cass `InterpreterLauncher`
** For now, only 2 implementation: `ShellScriptLauncher` & `SparkLauncher`. We could allow interpreter to specify its own launcher class, but it could be future work.
### What type of PR is it?
[Improvement | Refactoring]
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-2685
### How should this be tested?
Unit test is covered
### 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-2685
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2592.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 #2592
----
commit f54cb1d0a0f8bbf8cbd9a65ed33ceb9d8e1730c8
Author: Jeff Zhang <zj...@apache.org>
Date: 2017-08-31T12:21:04Z
ZEPPELIN-2685. Improvement on Interpreter class
----
---
[GitHub] zeppelin pull request #2592: ZEPPELIN-2685. Improvement on Interpreter class
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/2592
---
[GitHub] zeppelin issue #2592: ZEPPELIN-2685. Improvement on Interpreter class
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2592
It would not break the existing interpreter. I introduce `Launcher` concept to allow customize interpreter to launch. But for now all the interpreter still use `interpreter.sh` to launch it even for spark. But I have a plan to create a specific script to launch spark, because there's lots of specific logic for spark interpreter.
---
[GitHub] zeppelin issue #2592: ZEPPELIN-2685. Improvement on Interpreter class
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/2592
Launcher concept looks good to me.
---
[GitHub] zeppelin issue #2592: ZEPPELIN-2685. Improvement on Interpreter class
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2592
@zjffdu I don't have much concern about compatibility break you have mentioned. Left few comments, please check.
---
[GitHub] zeppelin issue #2592: ZEPPELIN-2685. Improvement on Interpreter class
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2592
Will merge it if no more discussion.
---
[GitHub] zeppelin issue #2592: ZEPPELIN-2685. Improvement on Interpreter class
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2592
@Leemoonsoo Do you have any concern about the compatibility break in this PR ?
* Add throw InterpreterException which is checked exception for the abstract methods of Interpreter, this would enforce the interpreter implementation to throw InterpreterException.
* field name refactoring.
** property -> properties
** getProperty() --> getProperties()
---
[GitHub] zeppelin issue #2592: ZEPPELIN-2685. Improvement on Interpreter class
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2592
@Leemoonsoo @jongyoul @felixcheung Could you help review it ? Thanks. It is followup of #2577
---