You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2022/01/31 03:29:10 UTC
[GitHub] [zeppelin] saLeox opened a new pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
saLeox opened a new pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294
### What is this PR for?
Optimize the interpreter execution process in the following prospect:
1. Close the interpreter executor thread when restart interpreter;
2. Only close opened interpreter when restart interpreter and create non-existent interpreter when create Interpreter;
3. Reuse the static innerScalaInterpreter class in the context of multi sub threads, loaded by the first spark interpreter;
### What type of PR is it?
[Improvement]
### Todos
* [ ] - Task
### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN-5644
### How should this be tested?
* Covered by existing unit test
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] saLeox commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
saLeox commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1029690969
Hi all, I add the unit tests for the features 1 to 3, pls help review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1069821006
CI is passed, will merge if no more comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1067658762
> @Reamer Hi, do you have time to have another round of review?
I'm afraid I can't do a code review here, as I'm not familiar with this part of the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu merged pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
zjffdu merged pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1067656146
@saLeox Sorry for late response, just a few minor comments
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu merged pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
zjffdu merged pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
jongyoul commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1025402712
@saLeox Thank you for your contribution. BTW, could you please add some tests for this feature? I think your implementation looks reasonable but it would be better we have tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] saLeox commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
saLeox commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1067690614
@zjffdu Thanks for the comments, I will commit and rebase again accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] saLeox commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
saLeox commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1067650086
@zjffdu Hi, any further feedback or concern regarding this PR? The integrated test seems running well after the rebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] saLeox commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
saLeox commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1069213561
@zjffdu Done, and pls help approve the new CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1048618564
@saLeox CI issue is fixed, could you rebase this PR to the latest master?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] saLeox commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
saLeox commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1047456505
@Reamer Hi, do you have time to have another round of review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#discussion_r795378350
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java
##########
@@ -482,9 +490,14 @@ public void close(String sessionId, String className) throws InterpreterRPCExcep
Iterator<Interpreter> it = interpreters.iterator();
while (it.hasNext()) {
Interpreter inp = it.next();
- if (inp.getClassName().equals(className)) {
+ LazyOpenInterpreter lazy = (LazyOpenInterpreter) inp;
Review comment:
Is this cast safe? I would prefer an Instanceof test before making this cast.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu closed pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
zjffdu closed pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1047459936
@saLeox Sorry for the late response, the CI is unstable recently. We are working on it https://github.com/apache/zeppelin/pull/4266
So we need to rerun the CI before merging this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1069194549
@saLeox Could you do a rebase and trigger CI again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] saLeox commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
saLeox commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1048620096
@zjffdu Sure, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] saLeox commented on pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
saLeox commented on pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#issuecomment-1025422297
> @saLeox Thank you for your contribution. BTW, could you please add some tests for this feature? I think your implementation looks reasonable but it would be better we have tests.
Hi @jongyoul, thanks for your reply, the modification I made are covered by some existing junit method, like SparkInterpreterTest.testSparkInterpreter() or RemoteInterpreterServerTest.testInterpreter(), should I add some specified cases in those methods?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#discussion_r826648694
##########
File path: spark/interpreter/src/main/java/org/apache/zeppelin/spark/SparkInterpreter.java
##########
@@ -68,6 +68,7 @@
private static AtomicInteger SESSION_NUM = new AtomicInteger(0);
private AbstractSparkScalaInterpreter innerInterpreter;
private Map<String, String> innerInterpreterClassMap = new HashMap<>();
+ private static Class innerInterpreterClazz;
Review comment:
Move it together with other static variables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#discussion_r826650446
##########
File path: spark/interpreter/src/test/java/org/apache/zeppelin/spark/SparkInterpreterTest.java
##########
@@ -17,6 +17,7 @@
package org.apache.zeppelin.spark;
+import com.google.common.reflect.ClassPath;
Review comment:
Not necessary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] saLeox commented on a change in pull request #4294: [ZEPPELIN-5644] Reduce redundancy during the interpreter execution process
Posted by GitBox <gi...@apache.org>.
saLeox commented on a change in pull request #4294:
URL: https://github.com/apache/zeppelin/pull/4294#discussion_r795385005
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java
##########
@@ -482,9 +490,14 @@ public void close(String sessionId, String className) throws InterpreterRPCExcep
Iterator<Interpreter> it = interpreters.iterator();
while (it.hasNext()) {
Interpreter inp = it.next();
- if (inp.getClassName().equals(className)) {
+ LazyOpenInterpreter lazy = (LazyOpenInterpreter) inp;
Review comment:
Thanks for your suggestion, Reamer, I will add the condition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org