[SPARK-31593][SS] Remove unnecessary streaming query progress update #28391

uncleGen · 2020-04-28T10:06:45Z

What changes were proposed in this pull request?

Structured Streaming progress reporter will always report an empty progress when there is no new data. As design, we should provide progress updates every 10s (default) when there is no new data.

Before PR:

After PR:

Why are the changes needed?

Fixes a bug around incorrect progress report

Does this PR introduce any user-facing change?

Fixes a bug around incorrect progress report

How was this patch tested?

existing ut and manual test

uncleGen · 2020-04-28T10:08:39Z

sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/ProgressReporter.scala

    if (hasExecuted) {
      // Reset noDataEventTimestamp if we processed any data
-      lastNoExecutionProgressEventTime = Long.MinValue
+      lastNoExecutionProgressEventTime = triggerClock.getTimeMillis()


When reset "lastNoExecutionProgressEventTime =Long.MinValue", it will make "now - noDataProgressEventInterval >= lastNoExecutionProgressEventTime" always true when there is no new data. Then progress reporter will report an empty progress.

uncleGen · 2020-04-28T10:09:21Z

cc @xuanyuanking

SparkQA · 2020-04-28T13:37:29Z

Test build #121989 has finished for PR 28391 at commit 4201180.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2020-04-29T00:33:13Z

This was actually proposed in #28040 but indicated UT failure hence abandoned. Happy to see it addressed, but looks like you'll also need to deal with UT failure. (UT failure is relevant.)

xuanyuanking · 2020-04-29T00:56:29Z

I think the change here is reasonable since the condition changed from hasNewData to hasExecuted in SPARK-31278. But yes, as @HeartSaVioR mentioned the UT is relevant. Maybe we also need to add new UT in StreamingQueryListenerSuite.scala?

uncleGen · 2020-04-29T01:45:58Z

@HeartSaVioR @xuanyuanking Thanks for reviewing, let me add ut.

SparkQA · 2020-05-09T04:40:50Z

Test build #122460 has finished for PR 28391 at commit 4a55b25.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-05-09T07:05:02Z

Test build #122461 has finished for PR 28391 at commit 914188c.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

xuanyuanking · 2020-05-09T07:28:12Z

retest this please

SparkQA · 2020-05-09T11:44:26Z

Test build #122462 has finished for PR 28391 at commit 914188c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-05-09T12:26:48Z

Test build #122464 has finished for PR 28391 at commit c3db6cf.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

xuanyuanking

LGTM, just some nits of comments.

xuanyuanking · 2020-05-12T07:02:44Z

sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingDeduplicationSuite.scala

+          noDataBatchEnableKey -> flag.toString,
+          // set `STREAMING_NO_DATA_PROGRESS_EVENT_INTERVAL` a small value to
+          // report an `empty` progress when no data come.
+          noDataProgressIntervalKey -> "1")


The way for fixing the failed UTs seems a little flaky. As your approach deletes empty progress generally, so here you set STREAMING_NO_DATA_PROGRESS_EVENT_INTERVAL to a small enough value to generate an empty one as expected, I think we can do better in either way:

Add comments to explain how small STREAMING_NO_DATA_PROGRESS_EVENT_INTERVAL needs to be set here is enough, seems less than 10 seconds is OK?

Delete the empty progress checking with explanation in comments.

Let's either use the manual clock or fix the UT like the way it doesn't depend on STREAMING_NO_DATA_PROGRESS_EVENT_INTERVAL.

For latter like we can set noDataProgressIntervalKey to 1000000 and remove the last assertion, which isn't actually testing the behavior of deduplication. Even better, you can still keep the last assertion for only when flag == true, which has a meaning of verification that state cleanup in empty input batch doesn't produce new outputs.

SPARK-31928 is filed to address the flakiness of this test on the master branch - that said, if we can make sure this test become no longer flaky, that should be nice.

xuanyuanking · 2020-05-13T06:11:31Z

cc @zsxwing @brkyvz @tdas

HyukjinKwon · 2020-05-28T08:58:09Z

retest this please

SparkQA · 2020-05-28T13:38:55Z

Test build #123227 has finished for PR 28391 at commit c3db6cf.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingQueryListenerSuite.scala

uncleGen · 2020-06-03T08:00:47Z

sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingQueryListenerSuite.scala

+        StartStream(
+          Trigger.ProcessingTime(100),
+          triggerClock = clock,
+          Map(noDataProgressIntervalKey -> "100")),


set "noDataProgressIntervalKey" to 100. After advance manual clock at batch 3 (with no data), it will report an empty progress event.

uncleGen · 2020-06-03T08:01:19Z

sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingQueryListenerSuite.scala

+      spark.streams.addListener(listener)
+
+      def checkProgressEvent(count: Int): StreamAction = {
+        AssertOnQuery { _ =>


add an inner function to simplify test.

SparkQA · 2020-06-03T13:54:53Z

Test build #123471 has finished for PR 28391 at commit 8479341.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR

LGTM

uncleGen · 2020-06-04T06:14:45Z

@HeartSaVioR @xuanyuanking Thanks for reviewing.

HeartSaVioR · 2020-06-08T23:15:52Z

cc. @tdas @zsxwing @jose-torres

gaborgsomogyi

LGTM

HyukjinKwon · 2020-06-12T04:51:52Z

I am going to merge this in few days if there's no more comments given that multiple LGTMs from the community. Let me know if there's any concern @tdas @zsxwing @jose-torres.

HyukjinKwon · 2020-06-12T04:52:32Z

retest this please

uncleGen · 2020-06-12T06:12:50Z

retest this please.

SparkQA · 2020-06-12T07:05:02Z

Test build #123888 has finished for PR 28391 at commit 8479341.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-12T07:05:02Z

Test build #123893 has finished for PR 28391 at commit 8479341.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

gaborgsomogyi · 2020-06-12T07:30:50Z

retest this please

SparkQA · 2020-06-12T14:25:30Z

Test build #123901 has finished for PR 28391 at commit 8479341.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-06-14T05:48:44Z

Merged to master and branch-3.0.

### What changes were proposed in this pull request? Structured Streaming progress reporter will always report an `empty` progress when there is no new data. As design, we should provide progress updates every 10s (default) when there is no new data. Before PR: ![20200428175008](https://user-images.githubusercontent.com/7402327/80474832-88a8ca00-897a-11ea-820b-d4be6127d2fe.jpg) ![20200428175037](https://user-images.githubusercontent.com/7402327/80474844-8ba3ba80-897a-11ea-873c-b7137bd4a804.jpg) ![20200428175102](https://user-images.githubusercontent.com/7402327/80474848-8e061480-897a-11ea-806e-28c6bbf1fe03.jpg) After PR: ![image](https://user-images.githubusercontent.com/7402327/80475099-f35a0580-897a-11ea-8fb3-53f343df2c3f.png) ### Why are the changes needed? Fixes a bug around incorrect progress report ### Does this PR introduce any user-facing change? Fixes a bug around incorrect progress report ### How was this patch tested? existing ut and manual test Closes #28391 from uncleGen/SPARK-31593. Authored-by: uncleGen <[email protected]> Signed-off-by: HyukjinKwon <[email protected]> (cherry picked from commit 1e40bcc) Signed-off-by: HyukjinKwon <[email protected]>

### What changes were proposed in this pull request? Structured Streaming progress reporter will always report an `empty` progress when there is no new data. As design, we should provide progress updates every 10s (default) when there is no new data. Before PR: ![20200428175008](https://user-images.githubusercontent.com/7402327/80474832-88a8ca00-897a-11ea-820b-d4be6127d2fe.jpg) ![20200428175037](https://user-images.githubusercontent.com/7402327/80474844-8ba3ba80-897a-11ea-873c-b7137bd4a804.jpg) ![20200428175102](https://user-images.githubusercontent.com/7402327/80474848-8e061480-897a-11ea-806e-28c6bbf1fe03.jpg) After PR: ![image](https://user-images.githubusercontent.com/7402327/80475099-f35a0580-897a-11ea-8fb3-53f343df2c3f.png) ### Why are the changes needed? Fixes a bug around incorrect progress report ### Does this PR introduce any user-facing change? Fixes a bug around incorrect progress report ### How was this patch tested? existing ut and manual test Closes apache#28391 from uncleGen/SPARK-31593. Authored-by: uncleGen <[email protected]> Signed-off-by: HyukjinKwon <[email protected]> (cherry picked from commit 1e40bcc) Signed-off-by: HyukjinKwon <[email protected]>

Remove unnecessary streaming query progress update

4201180

probot-autolabeler bot added SQL STRUCTURED STREAMING labels Apr 28, 2020

uncleGen commented Apr 28, 2020

View reviewed changes

update ut

4a55b25

fix codestyle

c3db6cf

uncleGen force-pushed the SPARK-31593 branch from 914188c to c3db6cf Compare May 9, 2020 08:05

uncleGen mentioned this pull request May 11, 2020

[SPARK-31642] Add Pagination Support for Structured Streaming Page #28485

Closed

xuanyuanking approved these changes May 12, 2020

View reviewed changes

HeartSaVioR reviewed May 31, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingQueryListenerSuite.scala Outdated Show resolved Hide resolved

fix comments

8479341

uncleGen commented Jun 3, 2020

View reviewed changes

HeartSaVioR approved these changes Jun 4, 2020

View reviewed changes

gaborgsomogyi approved these changes Jun 10, 2020

View reviewed changes

HyukjinKwon closed this in 1e40bcc Jun 14, 2020

[SPARK-31593][SS] Remove unnecessary streaming query progress update #28391

[SPARK-31593][SS] Remove unnecessary streaming query progress update #28391

Uh oh!

Conversation

uncleGen commented Apr 28, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

uncleGen Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

uncleGen commented Apr 28, 2020

Uh oh!

SparkQA commented Apr 28, 2020

Uh oh!

HeartSaVioR commented Apr 29, 2020

Uh oh!

xuanyuanking commented Apr 29, 2020

Uh oh!

uncleGen commented Apr 29, 2020

Uh oh!

SparkQA commented May 9, 2020

Uh oh!

SparkQA commented May 9, 2020

Uh oh!

xuanyuanking commented May 9, 2020

Uh oh!

SparkQA commented May 9, 2020

Uh oh!

SparkQA commented May 9, 2020

Uh oh!

xuanyuanking left a comment

Choose a reason for hiding this comment

Uh oh!

xuanyuanking May 12, 2020

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR May 31, 2020 • 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!

HeartSaVioR Jun 8, 2020

Choose a reason for hiding this comment

Uh oh!

xuanyuanking commented May 13, 2020

Uh oh!

HyukjinKwon commented May 28, 2020

Uh oh!

SparkQA commented May 28, 2020

Uh oh!

Uh oh!

uncleGen Jun 3, 2020 • 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!

uncleGen Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 3, 2020

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

uncleGen commented Jun 4, 2020

Uh oh!

HeartSaVioR commented Jun 8, 2020

Uh oh!

gaborgsomogyi left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jun 12, 2020

Uh oh!

HyukjinKwon commented Jun 12, 2020

Uh oh!

uncleGen commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

HeartSaVioR May 31, 2020 •

edited

Loading

uncleGen Jun 3, 2020 •

edited

Loading