[ZEPPELIN-2355] Livy cancel enhancements#2223
[ZEPPELIN-2355] Livy cancel enhancements#2223benoyantony wants to merge 4 commits intoapache:masterfrom
Conversation
|
@zjffdu , Could you please review ? |
benoyantony
force-pushed
the
livy-cancel-enhancement
branch
from
4f1f243 to
9fc6dbf
Compare
|
@benoyantony Thanks for the contribution, just left some comments. And could you add test for cancellation in LivySparkSQLInterpreter |
|
@zjffdu , Will try to add the unit test for cancellation in LivySparkSQLInterpreter. Any suggestion on how to simulate a sleep in SQL ? |
| } else { | ||
| LOGGER.warn("cancel is not supported for this version of livy: " + livyVersion); | ||
| } | ||
| paragraphsToCancel.add(context.getParagraphId()); |
There was a problem hiding this comment.
put it in if block to make the adding operation consistent with remove
There was a problem hiding this comment.
Since livyVersion is available only after the session is initialized, putting it inside the if block will work based on whether livyVersion is available or not. In some cases, this , might even throw NullPointerException.
I think , it's better to simply add it and then remove it later.
If the livyVersion supports cancellation, we will cancel and remove.
If not supported, we will warn and remove.
There was a problem hiding this comment.
Good catch, we can clear paragraphsToCancel if it is not supported.
There was a problem hiding this comment.
Good catch, we can clear paragraphsToCancel later if it is not supported.
| try { | ||
| LOGGER.info("Cancelling statement " + id); | ||
| cancelStatement(id); | ||
| paragraphsToCancel.remove(paragraphId); |
There was a problem hiding this comment.
It it better to put it in finally block to ensure it is removed.
There was a problem hiding this comment.
I have put the removal in finally block so that paragraph is removed from the set even if cancellation is not supported in the livy version.
|
@benoyantony You can call cancel in a separate thread, the same as testSparkInterpreterRDD, let me know if you have any other questions. |
…d moved the removal to finally block
|
A testcase errored out and it seems to be unrelated. |
| }; | ||
| cancelThread.start(); | ||
| boolean bCancelled = false; | ||
| for (int i=0; i <1000; i++) { |
There was a problem hiding this comment.
Why to iterate 1000 times ?
There was a problem hiding this comment.
@zjffdu , I do not see a way to sleep inside a sql statement. So the approach was to run up to 1000 sql statements while CancelThread will call cancel on the paragraph. If the cancel works, then at least one of the statements will be cancelled and the test will be successful.
Please let me know if there is a better approach.
There was a problem hiding this comment.
hmm, I think you don't need to run cancel in a separated thread. You can just call it as following to test. And don't use select count(1) from table_name instead, as show tables might not invoke spark jobs.
sqlInterpreter.cancel(context);
result = sqlInterpreter.interpret("select count(1) from table_name", context);
There was a problem hiding this comment.
This makes sense. I have modified the test case to make above call. Still calling the cancel on a separate thread to simulate a real scenario and to make it similar to other test cases.
benoyantony
force-pushed
the
livy-cancel-enhancement
branch
from
edcbd72 to
244e6d3
Compare
|
Thanks @benoyantony will merge it if no more comments. |
|
Thanks @zjffdu for reviewing and committing. |
2 participants
What is this PR for?
The Cancel functionality for the Livy interpreter has few issues. One issue is because a variable is not published correctly. Second issue is observed when there is a delay in launching the application. Any cancel before application launch is ignored. The third issue is that Cancel is not correctly implemented for SparkSQLInterpreter.
What type of PR is it?
Bug Fix
What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-2355
How should this be tested?
The test cases are modified to test the changes.
Screenshots (if appropriate)
Questions: