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 2020/10/03 12:16:43 UTC
[GitHub] [zeppelin] olkkoti opened a new pull request #3933: Zeppelin 5077
olkkoti opened a new pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933
### What is this PR for?
If a paragraph fails on something other than exception in Zeppelin, notebook will continue running paragraphs after it. This is unintuitive and regression from previous version.
To fix it, return "false" from running a paragraph, if it the actual user code fails. Also, after a failing "run all" operation, we need to broadcast paragraph states to client, so they wont stay in pending state after a failed run.
### What type of PR is it?
Bug Fix
### Todos
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-5077
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#issuecomment-707619987
@olkkoti What is the latest travis CI link ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#discussion_r502304539
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -1488,8 +1488,15 @@ private void runAllParagraphs(NotebookSocket conn,
new TypeToken<List<Map<String, Object>>>() {
}.getType());
- getNotebookService().runAllParagraphs(noteId, paragraphs, getServiceContext(fromMessage),
- new WebSocketServiceCallback<Paragraph>(conn));
+ if (!getNotebookService().runAllParagraphs(noteId, paragraphs, getServiceContext(fromMessage),
+ new WebSocketServiceCallback<Paragraph>(conn))) {
+ Note note = getNotebookService().getNote(noteId, getServiceContext(fromMessage), new SimpleServiceCallback());
Review comment:
Could you add one comment to explain why we need to broadcast paragraphs to client ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#issuecomment-711094824
> Made a cleaner fix which does not seem to break existing functionality. Can't get Flink tests to pass though. :/
>
> https://travis-ci.org/github/olkkoti/zeppelin/builds/736283820
Thanks @olkkoti The flink tests are unrelated, +1
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] olkkoti commented on a change in pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
olkkoti commented on a change in pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#discussion_r502786725
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -1488,8 +1488,15 @@ private void runAllParagraphs(NotebookSocket conn,
new TypeToken<List<Map<String, Object>>>() {
}.getType());
- getNotebookService().runAllParagraphs(noteId, paragraphs, getServiceContext(fromMessage),
- new WebSocketServiceCallback<Paragraph>(conn));
+ if (!getNotebookService().runAllParagraphs(noteId, paragraphs, getServiceContext(fromMessage),
+ new WebSocketServiceCallback<Paragraph>(conn))) {
+ Note note = getNotebookService().getNote(noteId, getServiceContext(fromMessage), new SimpleServiceCallback());
Review comment:
Client sets all paragraph states to "pending". Now, if we stop running paragraphs when one fails, paragraphs after the failed one are still in state "pending" in client. Broadcasting them to client resets the states.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#issuecomment-709812270
@olkkoti There're still test fails.
```
Failed tests:
NotebookRestApiTest.testRunParagraphSynchronously:181 expected:<200> but was:<500>
NotebookServiceTest.testParagraphOperations:391 null
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] olkkoti commented on a change in pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
olkkoti commented on a change in pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#discussion_r502887035
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -1488,8 +1488,15 @@ private void runAllParagraphs(NotebookSocket conn,
new TypeToken<List<Map<String, Object>>>() {
}.getType());
- getNotebookService().runAllParagraphs(noteId, paragraphs, getServiceContext(fromMessage),
- new WebSocketServiceCallback<Paragraph>(conn));
+ if (!getNotebookService().runAllParagraphs(noteId, paragraphs, getServiceContext(fromMessage),
+ new WebSocketServiceCallback<Paragraph>(conn))) {
+ Note note = getNotebookService().getNote(noteId, getServiceContext(fromMessage), new SimpleServiceCallback());
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#discussion_r502794926
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -1488,8 +1488,15 @@ private void runAllParagraphs(NotebookSocket conn,
new TypeToken<List<Map<String, Object>>>() {
}.getType());
- getNotebookService().runAllParagraphs(noteId, paragraphs, getServiceContext(fromMessage),
- new WebSocketServiceCallback<Paragraph>(conn));
+ if (!getNotebookService().runAllParagraphs(noteId, paragraphs, getServiceContext(fromMessage),
+ new WebSocketServiceCallback<Paragraph>(conn))) {
+ Note note = getNotebookService().getNote(noteId, getServiceContext(fromMessage), new SimpleServiceCallback());
Review comment:
I mean to add comment in the code so that user can understand what this piece of code is doing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] olkkoti commented on pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
olkkoti commented on pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#issuecomment-709992586
Made a cleaner fix which does not seem to break existing functionality. Can't get Flink tests to pass though. :/
https://travis-ci.org/github/olkkoti/zeppelin/builds/736283820
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] asfgit closed pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] olkkoti commented on pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
olkkoti commented on pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#issuecomment-706672945
Test added.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#issuecomment-706075296
@olkkoti Thanks for the contribution, could you add unit for it or create a follow up ticket for unit test ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#issuecomment-706075296
@olkkoti Thanks for the contribution, could you add unit for it or create a follow up ticket for unit test ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#discussion_r502304539
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -1488,8 +1488,15 @@ private void runAllParagraphs(NotebookSocket conn,
new TypeToken<List<Map<String, Object>>>() {
}.getType());
- getNotebookService().runAllParagraphs(noteId, paragraphs, getServiceContext(fromMessage),
- new WebSocketServiceCallback<Paragraph>(conn));
+ if (!getNotebookService().runAllParagraphs(noteId, paragraphs, getServiceContext(fromMessage),
+ new WebSocketServiceCallback<Paragraph>(conn))) {
+ Note note = getNotebookService().getNote(noteId, getServiceContext(fromMessage), new SimpleServiceCallback());
Review comment:
Could you add one comment to explain why we need to broadcast paragraphs to client ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] olkkoti commented on pull request #3933: [ZEPPELIN-5077] Run all paragraphs does not stop on failing paragraph
Posted by GitBox <gi...@apache.org>.
olkkoti commented on pull request #3933:
URL: https://github.com/apache/zeppelin/pull/3933#issuecomment-707846602
https://travis-ci.org/github/olkkoti/zeppelin/builds/734680686
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org