[SPARK-41432][UI][SQL] Protobuf serializer for SparkPlanGraphWrapper #39164

techaddict · 2022-12-22T00:52:18Z

What changes were proposed in this pull request?

Add Protobuf serializer for SparkPlanGraphWrapper

Why are the changes needed?

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

Does this PR introduce any user-facing change?

No

How was this patch tested?

New UT

LuciferYang · 2022-12-22T02:13:23Z

sql/core/pom.xml

        </dependency>
      </dependencies>
    </profile>
+    <profile>


I think we should also shaded and relocation protobuf-java in sql module, like

spark/core/pom.xml

Lines 690 to 696 in 73593d8

<relocation>

<pattern>com.google.protobuf</pattern>

<shadedPattern>${spark.shade.packageName}.spark-core.protobuf</shadedPattern>

<includes>

<include>com.google.protobuf.**</include>

</includes>

</relocation>

Would you mind adding to this pr? This is some necessary initial work for sql module @techaddict

both pom.xml and SparkBuild.scala

for SBT we can refer to

spark/project/SparkBuild.scala

Lines 618 to 631 in 73593d8

lazy val settings = Seq(

// Setting version for the protobuf compiler. This has to be propagated to every sub-project

// even if the project is not using it.

PB.protocVersion := BuildCommons.protoVersion,

// For some reason the resolution from the imported Maven build does not work for some

// of these dependendencies that we need to shade later on.

libraryDependencies ++= {

Seq(

"com.google.protobuf" % "protobuf-java" % protoVersion % "protobuf"

)

},

(Compile / PB.targets) := Seq(

PB.gens.java -> (Compile / sourceManaged).value

),

spark/project/SparkBuild.scala

Lines 645 to 654 in 73593d8

) ++ {

val sparkProtocExecPath = sys.props.get("spark.protoc.executable.path")

if (sparkProtocExecPath.isDefined) {

Seq(

PB.protocExecutable := file(sparkProtocExecPath.get)

)

} else {

Seq.empty

}

}

Otherwise, GA task may can't compile proto file

The old protobuf deps in root pom causes so many troubles...

spark/pom.xml

Lines 821 to 831 in e56f31d



<dependency>

<groupId>com.google.protobuf</groupId>

<artifactId>protobuf-java</artifactId>

<version>${protobuf.hadoopDependency.version}</version>

<scope>${hadoop.deps.scope}</scope>

</dependency>

From the comments, maybe we can try to remove the old protobuf dependency. mesos uses the shaded-protobuf one now. Let me do some tests with different hadoop profiles first

Thanks for looking into it!

LuciferYang · 2022-12-22T08:53:59Z

Since this pr will include some initialization work of the sql module, I think we should promote this first to facilitate the submission of other pr related to the sql module.

gengliangwang · 2022-12-22T09:24:28Z

@techaddict could you focus on this one first?

techaddict · 2022-12-22T10:22:01Z

@LuciferYang @gengliangwang sounds good

codecov-commenter · 2022-12-22T11:24:35Z

Codecov Report

Merging #39164 (e56f31d) into master (e56f31d) will not change coverage.
The diff coverage is n/a.

❗ Current head e56f31d differs from pull request most recent head 25b63fa. Consider uploading reports for the commit 25b63fa to get more accurate results

@@           Coverage Diff           @@
##           master   #39164   +/-   ##
=======================================
  Coverage   76.50%   76.50%           
=======================================
  Files         249      249           
  Lines       61187    61187           
  Branches     9069     9069           
=======================================
  Hits        46811    46811           
  Misses      13097    13097           
  Partials     1279     1279

Flag	Coverage Δ
unittests	`76.48% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sql/core/pom.xml

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

techaddict · 2022-12-22T21:07:42Z

@gengliangwang @LuciferYang addressed all the comments

gengliangwang · 2022-12-22T22:53:19Z

@techaddict How about following #39139? In that way, we don't need to have another shading in the SQL module.

techaddict · 2022-12-22T23:53:11Z

@gengliangwang done

gengliangwang · 2022-12-23T08:29:43Z

@LuciferYang This PR passes without the protobuf-java dep. Probably there is no maven build in GA tests?

LuciferYang · 2022-12-23T08:32:34Z

Yes, GA only runs sbt tests now , while Maven only run build with Java 11 & 17, no tests.

LuciferYang · 2022-12-23T09:23:26Z

Run

gh pr checkout 39164  
mvn clean test -pl sql/core -am -Dtest=none -DwildcardSuites=org.apache.spark.status.protobuf.sql.KVStoreProtobufSerializerSuite

locally

[INFO] --- scala-maven-plugin:4.8.0:compile (scala-compile-first) @ spark-sql_2.12 ---
[INFO] Compiler bridge file: /Users/yangjie01/.sbt/1.0/zinc/org.scala-sbt/org.scala-sbt-compiler-bridge_2.12-1.8.0-bin_2.12.17__52.0-1.8.0_20221110T195421.jar
[INFO] compiler plugin: BasicArtifact(com.github.ghik,silencer-plugin_2.12.17,1.7.10,null)
[INFO] compiling 576 Scala sources and 75 Java sources to /spark-source/sql/core/target/scala-2.12/classes ...
[ERROR] [Error] /spark-source/sql/core/src/main/scala/org/apache/spark/status/protobuf/sql/SparkPlanGraphWrapperSerializer.scala:34: Class com.google.protobuf.GeneratedMessageV3 not found - continuing with a stub.
[ERROR] [Error] : Unable to locate class corresponding to inner class entry for Builder in owner com.google.protobuf.GeneratedMessageV3
[ERROR] [Error] /spark-source/sql/core/src/main/scala/org/apache/spark/status/protobuf/sql/SparkPlanGraphWrapperSerializer.scala:37: Class com.google.protobuf.GeneratedMessageV3 not found - continuing with a stub.
[ERROR] [Error] /spark-source/sql/core/src/main/scala/org/apache/spark/status/protobuf/sql/SparkPlanGraphWrapperSerializer.scala:40: Class com.google.protobuf.GeneratedMessageV3 not found - continuing with a stub.
[ERROR] [Error] /spark-source/sql/core/src/main/scala/org/apache/spark/status/protobuf/sql/SparkPlanGraphWrapperSerializer.scala:42: value toByteArray is not a member of org.apache.spark.status.protobuf.StoreTypes.SparkPlanGraphWrapper
[ERROR] [Error] : Unable to locate class corresponding to inner class entry for Builder in owner com.google.protobuf.GeneratedMessageV3
[ERROR] [Error] /spark-source/sql/core/src/main/scala/org/apache/spark/status/protobuf/sql/SparkPlanGraphWrapperSerializer.scala:58: Class com.google.protobuf.GeneratedMessageV3 not found - continuing with a stub.
[ERROR] [Error] /spark-source/sql/core/src/main/scala/org/apache/spark/status/protobuf/sql/SparkPlanGraphWrapperSerializer.scala:59: Class com.google.protobuf.GeneratedMessageV3 not found - continuing with a stub.
[ERROR] [Error] : Unable to locate class corresponding to inner class entry for Builder in owner com.google.protobuf.GeneratedMessageV3
[ERROR] [Error] : Unable to locate class corresponding to inner class entry for Builder in owner com.google.protobuf.GeneratedMessageV3
[ERROR] [Error] /spark-source/sql/core/src/main/scala/org/apache/spark/status/protobuf/sql/SparkPlanGraphWrapperSerializer.scala:94: Class com.google.protobuf.GeneratedMessageV3 not found - continuing with a stub.
[ERROR] [Error] : Unable to locate class corresponding to inner class entry for Builder in owner com.google.protobuf.GeneratedMessageV3
[ERROR] [Error] : Unable to locate class corresponding to inner class entry for Builder in owner com.google.protobuf.GeneratedMessageV3
[ERROR] 13 errors found

techaddict · 2022-12-23T14:04:12Z

@LuciferYang I see the issue, too; it looks like the way around is to add a maven dependency

techaddict · 2022-12-24T01:51:31Z

@gengliangwang @LuciferYang rebased this, and its ready for review

...re/src/main/scala/org/apache/spark/status/protobuf/sql/SparkPlanGraphWrapperSerializer.scala

…PlanGraphWrapper

...re/src/main/scala/org/apache/spark/status/protobuf/sql/SparkPlanGraphWrapperSerializer.scala

LuciferYang

only one comment

+1, LGTM

gengliangwang · 2022-12-28T23:27:20Z

Thanks, merging to master

[SPARK-41432][UI][SQL] Protobuf serializer for SparkPlanGraphWrapper

743cd10

github-actions bot added BUILD SQL labels Dec 22, 2022

LuciferYang reviewed Dec 22, 2022

View reviewed changes

LuciferYang mentioned this pull request Dec 22, 2022

[SPARK-41431][CORE][SQL][UI] Protobuf serializer for SQLExecutionUIData #39139

Closed

Merge branch 'master' into SPARK-41432-SparkPlanGraphWrapper

4e6f9b2

add mvn and sbt config

25b63fa

LuciferYang reviewed Dec 22, 2022

View reviewed changes

sql/core/pom.xml Outdated Show resolved Hide resolved

Address comments

558ad23

LuciferYang reviewed Dec 22, 2022

View reviewed changes

sql/core/src/main/protobuf/org/apache/spark/status/protobuf/store_types.proto Outdated Show resolved Hide resolved

Merge branch 'master' into SPARK-41432-SparkPlanGraphWrapper

0f9f767

address comments

f9b0c13

github-actions bot added CORE and removed BUILD labels Dec 22, 2022

add tests

72e658a

fix name/desc

c721cca

techaddict added 2 commits December 23, 2022 05:54

Merge branch 'master' into SPARK-41432-SparkPlanGraphWrapper

98e0ebb

fix maven build

ab51bef

github-actions bot added the BUILD label Dec 23, 2022

Merge branch 'master' into SPARK-41432-SparkPlanGraphWrapper

a34dd22

github-actions bot removed the BUILD label Dec 24, 2022

LuciferYang reviewed Dec 27, 2022

View reviewed changes

...re/src/main/scala/org/apache/spark/status/protobuf/sql/SparkPlanGraphWrapperSerializer.scala Outdated Show resolved Hide resolved

techaddict added 5 commits December 27, 2022 11:24

Merge remote-tracking branch 'upstream/master' into SPARK-41432-Spark…

49ef0ed

…PlanGraphWrapper

reuse SQLPlanMetricSerializer

ce8621d

Merge branch 'master' into SPARK-41432-SparkPlanGraphWrapper

4a8e9d3

Merge branch 'master' into SPARK-41432-SparkPlanGraphWrapper

f8ffa42

fix imports

7c0d6be

LuciferYang reviewed Dec 28, 2022

View reviewed changes

...re/src/main/scala/org/apache/spark/status/protobuf/sql/SparkPlanGraphWrapperSerializer.scala Outdated Show resolved Hide resolved

LuciferYang approved these changes Dec 28, 2022

View reviewed changes

address comment

29c4ef4

gengliangwang approved these changes Dec 28, 2022

View reviewed changes

gengliangwang closed this in a28f08d Dec 28, 2022

	<relocation>
	<pattern>com.google.protobuf</pattern>
	<shadedPattern>${spark.shade.packageName}.spark-core.protobuf</shadedPattern>
	<includes>
	<include>com.google.protobuf.**</include>
	</includes>
	</relocation>

	lazy val settings = Seq(
	// Setting version for the protobuf compiler. This has to be propagated to every sub-project
	// even if the project is not using it.
	PB.protocVersion := BuildCommons.protoVersion,
	// For some reason the resolution from the imported Maven build does not work for some
	// of these dependendencies that we need to shade later on.
	libraryDependencies ++= {
	Seq(
	"com.google.protobuf" % "protobuf-java" % protoVersion % "protobuf"
	)
	},
	(Compile / PB.targets) := Seq(
	PB.gens.java -> (Compile / sourceManaged).value
	),

	) ++ {
	val sparkProtocExecPath = sys.props.get("spark.protoc.executable.path")
	if (sparkProtocExecPath.isDefined) {
	Seq(
	PB.protocExecutable := file(sparkProtocExecPath.get)
	)
	} else {
	Seq.empty
	}
	}

	<!-- In theory we need not directly depend on protobuf since Spark does not directly
	use it. However, when building with Hadoop/YARN 2.2 Maven doesn't correctly bump
	the protobuf version up from the one Mesos gives. For now we include this variable
	to explicitly bump the version when building with YARN. It would be nice to figure
	out why Maven can't resolve this correctly (like SBT does). -->
	<dependency>
	<groupId>com.google.protobuf</groupId>
	<artifactId>protobuf-java</artifactId>
	<version>${protobuf.hadoopDependency.version}</version>
	<scope>${hadoop.deps.scope}</scope>
	</dependency>

[SPARK-41432][UI][SQL] Protobuf serializer for SparkPlanGraphWrapper #39164

[SPARK-41432][UI][SQL] Protobuf serializer for SparkPlanGraphWrapper #39164

Uh oh!

Conversation

techaddict commented Dec 22, 2022

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!

LuciferYang Dec 22, 2022 • 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!

LuciferYang Dec 22, 2022 • 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!

LuciferYang Dec 22, 2022 • 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!

gengliangwang Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

LuciferYang Dec 22, 2022 • 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!

gengliangwang Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Dec 22, 2022

Uh oh!

gengliangwang commented Dec 22, 2022

Uh oh!

techaddict commented Dec 22, 2022

Uh oh!

codecov-commenter commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

techaddict commented Dec 22, 2022

Uh oh!

gengliangwang commented Dec 22, 2022

Uh oh!

techaddict commented Dec 22, 2022

Uh oh!

gengliangwang commented Dec 23, 2022

Uh oh!

LuciferYang commented Dec 23, 2022

Uh oh!

LuciferYang commented Dec 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

techaddict commented Dec 23, 2022

Uh oh!

techaddict commented Dec 24, 2022

Uh oh!

Uh oh!

Uh oh!

LuciferYang left a comment • 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!

gengliangwang commented Dec 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LuciferYang Dec 22, 2022 •

edited

Loading

LuciferYang Dec 22, 2022 •

edited

Loading

LuciferYang Dec 22, 2022 •

edited

Loading

LuciferYang Dec 22, 2022 •

edited

Loading

codecov-commenter commented Dec 22, 2022 •

edited

Loading

LuciferYang commented Dec 23, 2022 •

edited

Loading

LuciferYang left a comment •

edited

Loading