[SPARK-41677][CORE][SQL][SS][UI] Add Protobuf serializer for `StreamingQueryProgressWrapper`#39642

Closed

LuciferYang wants to merge 19 commits intoapache:masterfrom

LuciferYang:SPARK-41677-2

Contributor

LuciferYang commented Jan 18, 2023 •

edited

Loading

What changes were proposed in this pull request?

Add Protobuf serializer for StreamingQueryProgressWrapper

Why are the changes needed?

Support fast and compact serialization/deserialization for StreamingQueryProgressWrapper over RocksDB.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Add new UT
Manual test sql module with LIVE_UI_LOCAL_STORE_DIR, all test passed:

build/mvn clean install -DskipTests -pl sq/core -am
export LIVE_UI_LOCAL_STORE_DIR=/tmp/spark-ui
build/mvn clean install -pl sql/core

There 4 existing test suites classes use StreamingQueryProgressWrapper:

StateStoreCoordinatorSuite
StreamingQueryStatusListenerWithDiskStoreSuite
UISeleniumSuite
UISeleniumWithRocksDBBackendSuite


          support StreamingQueryProgressWrapper

eab4186

LuciferYang marked this pull request as draft

January 18, 2023 12:17

LuciferYang changed the title ~~[SPARK-41677][CORE][SQL][SS][UI] Add Protobuf serializer for StreamingQueryProgressWrapper~~ [WIP][SPARK-41677][CORE][SQL][SS][UI] Add Protobuf serializer for StreamingQueryProgressWrapper

github-actions bot added CORE SQL STRUCTURED STREAMING WEB UI labels

LuciferYang added 2 commits

January 19, 2023 10:56


          Merge branch 'upmaster' into SPARK-41677-2

3e6ade0


          add comments

fbbecf0

LuciferYang commented

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala

                 val durationMs: ju.Map[String, JLong],
+                // If `eventTime` is no longer read-only, need change
+                // `StreamingQueryProgressSerializer#deserialize` to return mutable Java Map
                 val eventTime: ju.Map[String, String],

Contributor Author

LuciferYang Jan 19, 2023

There are 6 ju.Map, durationMs and observedMetrics already use mutable Java Map, the remaining 4 Java Map should be read-only at present, but for safety, we can also directly return mutable Java Map in the deserialize method

Contributor Author

LuciferYang Jan 20, 2023

For safety, 6 ju.Map are assigned as mutable Map after 8e89213


          return JHashMap

8e89213

LuciferYang marked this pull request as ready for review

January 20, 2023 02:07

LuciferYang changed the title ~~[WIP][SPARK-41677][CORE][SQL][SS][UI] Add Protobuf serializer for StreamingQueryProgressWrapper~~ [SPARK-41677][CORE][SQL][SS][UI] Add Protobuf serializer for StreamingQueryProgressWrapper

LuciferYang added 2 commits

January 20, 2023 11:01


          Merge branch 'upmaster' into SPARK-41677-2

d3f9ac0


          Merge branch 'upmaster' into SPARK-41677-2

6fe0163

LuciferYang marked this pull request as draft

January 20, 2023 05:02

Contributor Author

LuciferYang commented Jan 20, 2023

Will refactor after #39666 merged

LuciferYang added 3 commits

January 21, 2023 15:13


          Merge branch 'upmaster' into SPARK-41677-2

3680d8b


          handle null string

198fcb2


          id, runId to optional

LuciferYang mentioned this pull request

[SPARK-42146][CORE] Refactor Utils#setStringField to make maven build pass when sql module use this method #39688

Closed


          Merge branch 'apache:master' into SPARK-41677-2

c9275f6

Contributor Author

LuciferYang commented Jan 22, 2023

rebased

LuciferYang marked this pull request as ready for review

January 22, 2023 03:53

Contributor Author

LuciferYang commented Jan 22, 2023

cc @gengliangwang

Does this one have a chance to Spark 3.4.0 ? Or wait for the next version?

gengliangwang reviewed

View reviewed changes

core/src/main/protobuf/org/apache/spark/status/protobuf/store_types.proto

+                int64 num_rows_dropped_by_watermark = 9;
+                int64 num_shuffle_partitions = 10;
+                int64 num_state_store_instances = 11;
+                map<string, int64> custom_metrics = 12;

Member

gengliangwang Jan 22, 2023

I am concerned about the nullability of all these maps. Shall we check/test all of them?

Contributor Author

LuciferYang Jan 23, 2023

a904a27 check/test all map

699ebd1 add setJMapField function to Utils

LuciferYang added 3 commits

January 23, 2023 10:57


          Merge branch 'upmaster' into SPARK-41677-2

9b3c19a


          check null and test null map

a904a27


          Merge branch 'SPARK-41677-2' of github.com:LuciferYang/spark into SPA…

7566de4

…RK-41677-2

LuciferYang commented

View reviewed changes

...ore/src/test/scala/org/apache/spark/status/protobuf/sql/KVStoreProtobufSerializerSuite.scala

                 }
+                test("StreamingQueryProgressWrapper") {
+                  val normalInput = {

Contributor Author

LuciferYang Jan 23, 2023

Two objects are manually created due to many fields can be null

LuciferYang commented

View reviewed changes

...ore/src/test/scala/org/apache/spark/status/protobuf/sql/KVStoreProtobufSerializerSuite.scala

+                    assert(progress.batchId == resultProcess.batchId)
+                    assert(progress.batchDuration == resultProcess.batchDuration)
+                    if (hasNullValue) {
+                      assert(resultProcess.durationMs.isEmpty)

Contributor Author

LuciferYang Jan 23, 2023

null map input return empty map

LuciferYang added 2 commits

January 23, 2023 12:29


          setJMap

699ebd1


          rename f

061275c

github-actions bot added the BUILD label


          revert java version

e578627

github-actions bot removed the BUILD label

gengliangwang reviewed

View reviewed changes

...e/src/main/scala/org/apache/spark/status/protobuf/sql/StreamingQueryProgressSerializer.scala Show resolved Hide resolved

gengliangwang reviewed

View reviewed changes

...e/src/main/scala/org/apache/spark/status/protobuf/sql/StreamingQueryProgressSerializer.scala Show resolved Hide resolved

gengliangwang reviewed

View reviewed changes

...e/src/main/scala/org/apache/spark/status/protobuf/sql/StreamingQueryProgressSerializer.scala Outdated

Member

gengliangwang Jan 24, 2023

use setJMapField?


          use setJMapField

81e39c8

LuciferYang commented

View reviewed changes

...e/src/main/scala/org/apache/spark/status/protobuf/sql/StreamingQueryProgressSerializer.scala Outdated

Contributor Author

LuciferYang Jan 25, 2023

use setJMapField need to define an additional function or use an anonymous function due this is not a simple putAll scenario.

Any other better way to write this ? @gengliangwang


          to a class private function

480f48b

LuciferYang commented

View reviewed changes

...e/src/main/scala/org/apache/spark/status/protobuf/sql/StreamingQueryProgressSerializer.scala

+                  )
+                }
+                private def putAllObservedMetrics(

Contributor Author

LuciferYang Jan 25, 2023

Change to define a class level private function

gengliangwang reviewed

View reviewed changes

...e/src/main/scala/org/apache/spark/status/protobuf/sql/StreamingQueryProgressSerializer.scala

+                    builder: StoreTypes.StreamingQueryProgress.Builder,
+                    observedMetrics: JMap[String, Row]): Unit = {
+                  observedMetrics.forEach {
+                    case (k, v) => builder.putObservedMetrics(k, mapper.writeValueAsString(v))

Member

gengliangwang Jan 25, 2023

nit: Let's add one line comment to mention why we choose to encode the row object with json.

gengliangwang approved these changes

View reviewed changes

Member

gengliangwang left a comment

LGTM. Thanks for working on this during holiday!


          add comments

6534fad

Contributor Author

LuciferYang commented Jan 25, 2023

done. Thank you for reviewing the code during the holiday :) @gengliangwang

Contributor Author

LuciferYang commented Jan 25, 2023

GA passed

gengliangwang pushed a commit that referenced this pull request


          [SPARK-41677][CORE][SQL][SS][UI] Add Protobuf serializer for `Streami…

1a9cacb

…ngQueryProgressWrapper`

### What changes were proposed in this pull request?
Add Protobuf serializer for `StreamingQueryProgressWrapper `

### Why are the changes needed?
Support fast and compact serialization/deserialization for `StreamingQueryProgressWrapper` over RocksDB.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
- Add new UT
- Manual test sql module with `LIVE_UI_LOCAL_STORE_DIR`, all test passed:

```
build/mvn clean install -DskipTests -pl sq/core -am
export LIVE_UI_LOCAL_STORE_DIR=/tmp/spark-ui
build/mvn clean install -pl sql/core
```

There 4 existing test suites classes use `StreamingQueryProgressWrapper`:
 - StateStoreCoordinatorSuite
 - StreamingQueryStatusListenerWithDiskStoreSuite
 - UISeleniumSuite
 - UISeleniumWithRocksDBBackendSuite

Closes #39642 from LuciferYang/SPARK-41677-2.

Lead-authored-by: yangjie01 <yangjie01@baidu.com>
Co-authored-by: YangJie <yangjie01@baidu.com>
Signed-off-by: Gengliang Wang <gengliang@apache.org>
(cherry picked from commit 7b93415)
Signed-off-by: Gengliang Wang <gengliang@apache.org>

gengliangwang closed this in

7b93415

Member

gengliangwang commented Jan 25, 2023

Thanks, merged to master & branch-3.4

Contributor Author

LuciferYang commented Jan 25, 2023

Thanks @gengliangwang

TarunParimi pushed a commit to TarunParimi/spark that referenced this pull request


          [SPARK-41677][CORE][SQL][SS][UI] Add Protobuf serializer for `Streami…

7df245e

…ngQueryProgressWrapper`

Add Protobuf serializer for `StreamingQueryProgressWrapper `

Support fast and compact serialization/deserialization for `StreamingQueryProgressWrapper` over RocksDB.

No

- Add new UT
- Manual test sql module with `LIVE_UI_LOCAL_STORE_DIR`, all test passed:

```
build/mvn clean install -DskipTests -pl sq/core -am
export LIVE_UI_LOCAL_STORE_DIR=/tmp/spark-ui
build/mvn clean install -pl sql/core
```

There 4 existing test suites classes use `StreamingQueryProgressWrapper`:
 - StateStoreCoordinatorSuite
 - StreamingQueryStatusListenerWithDiskStoreSuite
 - UISeleniumSuite
 - UISeleniumWithRocksDBBackendSuite

Closes apache#39642 from LuciferYang/SPARK-41677-2.

Lead-authored-by: yangjie01 <yangjie01@baidu.com>
Co-authored-by: YangJie <yangjie01@baidu.com>
Signed-off-by: Gengliang Wang <gengliang@apache.org>
(cherry picked from commit 7b93415)
Signed-off-by: Gengliang Wang <gengliang@apache.org>

TarunParimi pushed a commit to TarunParimi/spark that referenced this pull request


          [SPARK-41677][CORE][SQL][SS][UI] Add Protobuf serializer for `Streami…

46ef32f

…ngQueryProgressWrapper`

Add Protobuf serializer for `StreamingQueryProgressWrapper `

Support fast and compact serialization/deserialization for `StreamingQueryProgressWrapper` over RocksDB.

No

- Add new UT
- Manual test sql module with `LIVE_UI_LOCAL_STORE_DIR`, all test passed:

```
build/mvn clean install -DskipTests -pl sq/core -am
export LIVE_UI_LOCAL_STORE_DIR=/tmp/spark-ui
build/mvn clean install -pl sql/core
```

There 4 existing test suites classes use `StreamingQueryProgressWrapper`:
 - StateStoreCoordinatorSuite
 - StreamingQueryStatusListenerWithDiskStoreSuite
 - UISeleniumSuite
 - UISeleniumWithRocksDBBackendSuite

Closes apache#39642 from LuciferYang/SPARK-41677-2.

Lead-authored-by: yangjie01 <yangjie01@baidu.com>
Co-authored-by: YangJie <yangjie01@baidu.com>
Signed-off-by: Gengliang Wang <gengliang@apache.org>
(cherry picked from commit 7b93415)
Signed-off-by: Gengliang Wang <gengliang@apache.org>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CORE SQL STRUCTURED STREAMING WEB UI