Buffer append output results + fix extra incorrect results #1283

beriaanirudh · 2016-08-04T12:40:24Z

What is this PR for?

There are 2 issues and their proposed fixes:

On a paragraph run, for every line of output, there is a broadcast of the new line from zeppelin. In case of thousands of lines of output, the browser/s would hang because of the volume of these append-output events.
In the above case, besides the browser-hang, another bug observed is that result data is will repeated twice (coming from append-output calls + finish-event calls).

The proposed solution for #1 is:

Buffer the append-output event into a queue instead of sending the event immediately.
In a separate thread, read from the queue periodically and send the append-output event.

Solution for #2 is:

Donot append output to result if the paragraph is not runnig.

What type of PR is it?

Improvement + Bug Fix

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1292

How should this be tested?

The test could be to run a simple paragraph with large result. Eg:

%sh
for i in {1..10000}
do
echo $i
done

PS: One will need to clear browser cache between running with and without this code patch since there are javascript changes as well.

Screenshots (if appropriate)

Questions:

Does the licenses files need update?
No
Is there breaking changes for older versions?
No
Does this needs documentation?
It could need for the design. Otherwise I have added code comments explaining behaviour.

corneadoug · 2016-08-04T13:34:19Z

zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js

+     * could be called in PENDING state as well
+     */
+    if ($scope.paragraph.id === data.paragraphId &&
+       ($scope.paragraph.status === 'RUNNING' || $scope.paragraph.status === 'PENDING')) {


Considering the Comment above, why would we accept PENDING state?

What I observed was that in between the pending and the running state, some (I observed 1) append-output events were called. If we miss those, then while the paragraph is running, we would see that some intial-result line/s is/are missing.
Also, when the para execution is finished, the complete note is sent again (with results), so we would see the correct result.

I see, the comment was a bit misleading, as it seems it was saying that it could be errorneously call es as well in PENDING state.

Ya, when I re-read it sounds that way.. Re-phrased it.

jongyoul · 2016-08-05T05:18:20Z

...lin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/AppendOutputRunner.java

+          + " and send paragraph append data");
+      thread = new Thread(new AppendOutputRunner());
+      thread.start();
+    }


That's not thread-safe. You should use synchronized or something else

jongyoul · 2016-08-05T05:21:33Z

I think you'd better use ScheduledExecutorService by running separate thread because AppendOutputRunner always execute while(true) in your current implementation.

jongyoul · 2016-08-05T05:32:41Z

There're some Thread.sleep(1000) in your tests. In general, it would be nice and never be a blocker. It, however, occurs build error if CI become extremely slow. It also become a flaky test. Do you have any idea to make it more stable? And I have a question. Which part prevents updating output twice?

beriaanirudh · 2016-08-05T06:43:42Z

It is easier to handle the twice output on the javascript side, so I have added the check in file paragraph.controller.js. The browser would not let results to be appended if the paragraph is completed. Also, since we are buffering events now, there would be few (1-2) events called after paragraph execution completion.
Working on your other comments..

1. Synchronize on AppendOutputRunner creation 2. Use ScheduledExecutorService instead of while loop 3. Remove Thread.sleep() from tests

beriaanirudh · 2016-08-06T15:50:45Z

@jongyoul: I have incorported all feedback:

Using ScheduledExecutorService for ensuring AppendOutputRunner is up, instead of doing so inside the while loop.
Synchronizing on the thread check and creation.
Removed all Thread.sleep() from unit tests except for one test "testClubbedData()" (which is basically a simulation of actual run). I am out of ideas for that one... Please advice.. We could remove that test as well..

jongyoul · 2016-08-08T08:07:52Z

...interpreter/src/test/java/org/apache/zeppelin/interpreter/remote/AppendOutputRunnerTest.java

+    prepareInvocationCounts(listener);
+    AppendOutputRunner.setListener(listener);
+    CheckAppendOutputRunner.startRunnerForUnitTests();
+    while(numInvocations != numTimes);


I suggest you add Circuit Breaker with few seconds to avoid running infinitely.

Done.. Added a check for to fail the tests after 2 seconds.. Would have to add CircuitBreaker in dependencies, so just using System times.

beriaanirudh · 2016-08-09T10:31:24Z

My tests needed some fixes to work with other tests. I have fixed them. But the build still has some errors I think not related to my change. https://travis-ci.org/apache/zeppelin/builds/150876902 .

beriaanirudh · 2016-08-11T04:17:55Z

@jongyoul @corneadoug I have incorporated all feedback, and added comments on queries. This PR is ready for review.

jongyoul · 2016-08-11T07:41:54Z

I've a few comment on your comments. Please check it.

corneadoug · 2016-08-16T04:43:39Z

Tested a few cases to check that the front-end still handle most of the cases, and its good

bzz · 2016-08-16T05:23:20Z

...lin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/AppendOutputRunner.java

+  private static final Long SAFE_PROCESSING_STRING_SIZE = new Long(100000);
+
+  private static final BlockingQueue<AppendOutputBuffer> QUEUE =
+      new LinkedBlockingQueue<AppendOutputBuffer>();


This dynamic data structure does not feel like a constant, so accouding to the project styleguide#s5.2.4-constant-names) better be rather named queue.

beriaanirudh · 2016-08-16T07:44:56Z

@corneadoug thanks for verifying.
@jongyoul Got it fixed now. I had mis-understood previously.

beriaanirudh · 2016-08-18T06:19:10Z

@corneadoug @jongyoul @bzz this is ready for review.

bzz · 2016-08-19T09:58:42Z

...lin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/AppendOutputRunner.java

+  public void run() {
+
+    Map<String, Map<String, StringBuilder> > noteMap =
+        new HashMap<String, Map<String, StringBuilder> >();


Probably a nitpick, but 'diamond operator' should come handy

Map<String, Map<String, StringBuilder> > noteMap = new HashMap<>();

beriaanirudh · 2016-08-22T07:08:20Z

@corneadoug @jongyoul @bzz this is ready for review.

jongyoul · 2016-08-22T15:55:14Z

@beriaanirudh You need to change more where you don't use 'diamond operator'. Can you fix them?

beriaanirudh · 2016-08-23T07:54:21Z

@jongyoul done. Used diamond operator at all places except for 1 where compiler wouldn't allow (AppendOutputRunner.java:78)

bzz · 2016-08-24T02:41:46Z

@beriaanirudh thank you for prompt fixes!

vipul1409 · 2016-08-24T10:33:10Z

+1 Looking forward to this

jongyoul · 2016-08-24T11:16:16Z

LGTM

beriaanirudh · 2016-08-25T09:00:47Z

@corneadoug @bzz this is ready for review...

Leemoonsoo · 2016-08-25T15:03:46Z

Text output streaming used to work in following way (branch-0.6)

But somehow, this PR (and master branch) works like

Shell we address this problem first, before apply this patch?

corneadoug · 2016-08-26T01:29:28Z

@Leemoonsoo

The behaviour is still the same with:

(1 to 40).foreach{ i=>
    Thread.sleep(1000)
    println("Hello " + i)
}

Leemoonsoo · 2016-08-26T03:49:04Z

@corneadoug yes, that streams output every one seconds if i try the master branch.

However, I did some more experiments and found

Output streaming : master branch (X), this pullrequest (X)

%sh
for i in {1..3}
do
    date
    sleep 2
done

Output streaming : master branch (O), this pullrequest (X)

%spark
(1 to 3).foreach{i=>
    Thread.sleep(2000)
    println(new java.util.Date)
}

@corneadoug @beriaanirudh Could you verify these cases, too?

corneadoug · 2016-08-26T07:49:21Z

I tested both cases:
Case1: master (X), this branch (X)
Case2: master (O), this branch(X)

However the first time I tried this branch and commented, the Case 2 was fine in this Branch

Leemoonsoo · 2016-08-27T17:03:19Z

According to the test results,

%sh interpreter output streaming might be other issue.
but it looks like %spark interpreter output streaming in this branch is not working as expected.

@beriaanirudh Could you take care of it?

beriaanirudh · 2016-08-29T13:55:24Z

Hey,
I had done the testing on streaming cases for shell and spark earlier while raising this PR, and they did work then. I just did the testing on top of d11221fb8af5568416ef5041fc2da8b6fa08598b (currently latest master) and I found these results:
Case1: master (X), this branch (X)
Case2: master(O), this branch (O)

I think SparkInterpreter could have been broken which got fixed today in 5f1208bd (ZEPPELIN-1284). Perhaps thats why Case2 was failing 2-3 days back. Could you please confirm my test-results?
cc @Leemoonsoo @corneadoug

corneadoug · 2016-08-30T01:40:54Z

Don't know why sometimes we get some failure in that branch,
Tried again and got:

Case1: master (X), this branch (X)
Case2: master (O), this branch(O)

beriaanirudh · 2016-08-30T09:56:16Z

Thanks for the verification @corneadoug . Even I am not able to reproduce it. My best guess is that it had to do something with the fix I mentioned above.

beriaanirudh · 2016-09-01T13:49:17Z

@corneadoug @Leemoonsoo please let me know if any action-item on my side.

Leemoonsoo · 2016-09-01T20:11:02Z

Tested again and got Case2 works fine on this branch.
LGTM. CI failure is irrelevant to this change.
Merge if there're no more discussions.

Thanks @beriaanirudh for the contribution!

There are 2 issues and their proposed fixes: 1. On a paragraph run, for every line of output, there is a broadcast of the new line from zeppelin. In case of thousands of lines of output, the browser/s would hang because of the volume of these append-output events. 2. In the above case, besides the browser-hang, another bug observed is that result data is will repeated twice (coming from append-output calls + finish-event calls). The proposed solution for apache#1 is: - Buffer the append-output event into a queue instead of sending the event immediately. - In a separate thread, read from the queue periodically and send the append-output event. Solution for apache#2 is: - Donot append output to result if the paragraph is not runnig. Improvement + Bug Fix https://issues.apache.org/jira/browse/ZEPPELIN-1292 The test could be to run a simple paragraph with large result. Eg: ``` %sh for i in {1..10000} do echo $i done ``` PS: One will need to clear browser cache between running with and without this code patch since there are javascript changes as well. * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? It could need for the design. Otherwise I have added code comments explaining behaviour. Author: Beria <beria@qubole.com> Closes apache#1283 from beriaanirudh/ZEPPELIN-1292 and squashes the following commits: 17f0524 [Beria] Use diamond operator 7852368 [Beria] nit 4b68c86 [Beria] fix checkstyle d168614 [Beria] Remove un-necessary class CheckAppendOutputRunner 2eae38e [Beria] Make AppendOutputRunner non-static 72c316d [Beria] Scheduler service to replace while loop in AppendOutputRunner 599281f [Beria] fix unit tests that run after dd24816 [Beria] Add license in test file 3984ef8 [Beria] fix tests when ran with other tests 1c893c0 [Beria] Add licensing 1bdd669 [Beria] fix javadoc comment 27790e4 [Beria] Avoid infinite loop in tests 5057bb3 [Beria] Incorporate feedback 1. Synchronize on AppendOutputRunner creation 2. Use ScheduledExecutorService instead of while loop 3. Remove Thread.sleep() from tests 82e9c4a [Beria] Fix comment 7020f0c [Beria] Buffer append output results + fix extra incorrect results (cherry picked from commit 11becde)

Buffer append output results + fix extra incorrect results

7020f0c

corneadoug reviewed Aug 4, 2016
View reviewed changes

Fix comment

82e9c4a

jongyoul reviewed Aug 5, 2016
View reviewed changes

Incorporate feedback

5057bb3

1. Synchronize on AppendOutputRunner creation 2. Use ScheduledExecutorService instead of while loop 3. Remove Thread.sleep() from tests

jongyoul reviewed Aug 8, 2016
View reviewed changes

Beria added 6 commits August 8, 2016 16:14

Avoid infinite loop in tests

27790e4

fix javadoc comment

1bdd669

Add licensing

1c893c0

fix tests when ran with other tests

3984ef8

Add license in test file

dd24816

fix unit tests that run after

599281f

bzz reviewed Aug 16, 2016
View reviewed changes

Scheduler service to replace while loop in AppendOutputRunner

72c316d

Beria added 3 commits August 16, 2016 22:08

Make AppendOutputRunner non-static

2eae38e

Remove un-necessary class CheckAppendOutputRunner

d168614

fix checkstyle

4b68c86

bzz reviewed Aug 19, 2016
View reviewed changes

nit

7852368

Use diamond operator

17f0524

asfgit closed this in 11becde Sep 3, 2016

Buffer append output results + fix extra incorrect results #1283

Buffer append output results + fix extra incorrect results #1283

Uh oh!

Conversation

beriaanirudh commented Aug 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jongyoul commented Aug 5, 2016

Uh oh!

jongyoul commented Aug 5, 2016

Uh oh!

beriaanirudh commented Aug 5, 2016

Uh oh!

beriaanirudh commented Aug 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beriaanirudh Aug 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beriaanirudh commented Aug 9, 2016

Uh oh!

beriaanirudh commented Aug 11, 2016

Uh oh!

jongyoul commented Aug 11, 2016

Uh oh!

corneadoug commented Aug 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beriaanirudh commented Aug 16, 2016

Uh oh!

beriaanirudh commented Aug 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beriaanirudh commented Aug 22, 2016

Uh oh!

jongyoul commented Aug 22, 2016

Uh oh!

beriaanirudh commented Aug 23, 2016

Uh oh!

bzz commented Aug 24, 2016

Uh oh!

vipul1409 commented Aug 24, 2016

Uh oh!

jongyoul commented Aug 24, 2016

Uh oh!

beriaanirudh commented Aug 25, 2016

Uh oh!

Leemoonsoo commented Aug 25, 2016

Uh oh!

corneadoug commented Aug 26, 2016

Uh oh!

Leemoonsoo commented Aug 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corneadoug commented Aug 26, 2016

beriaanirudh commented Aug 4, 2016 •

edited

Loading

beriaanirudh Aug 8, 2016 •

edited

Loading

Leemoonsoo commented Aug 26, 2016 •

edited

Loading