[SPARK-41768][CORE] Refactor the definition of enum to follow with the code style by panbingkun · Pull Request #39286 · apache/spark

panbingkun · 2022-12-29T11:40:41Z

What changes were proposed in this pull request?

The pr aims to refactor the definition of enum in UI protobuf serializer to follow with the code style.

Why are the changes needed?

Following code style:
https://developers.google.com/protocol-buffers/docs/style#enums

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA
Existed UT.

…atus` to follow with the code style

LuciferYang · 2022-12-30T07:10:28Z

core/src/main/scala/org/apache/spark/status/protobuf/JobExecutionStatusSerializer.scala

+  private def PREFIX = "JOB_EXECUTION_STATUS_"
+
+  private[protobuf] def serialize(input: JobExecutionStatus): StoreTypes.JobExecutionStatus = {
+    StoreTypes.JobExecutionStatus.valueOf(PREFIX + input.toString)


How much additional delay will string join and string remove bring?

or following as？

how about use a map to store the mapping relationships?

+1 for using pattern match here.

@panbingkun shall we refactor enum DeterministicLevel as well?

Done

…atus` to follow with the code style

panbingkun · 2022-12-31T02:21:12Z

cc @gengliangwang

AmplabJenkins · 2022-12-31T07:27:08Z

Can one of the admins verify this patch?

gengliangwang · 2023-01-01T00:07:13Z

@panbingkun shall we refactor enum DeterministicLevel as well?

…atus` to follow with the code style

LuciferYang · 2023-01-01T10:54:12Z

core/src/main/scala/org/apache/spark/status/protobuf/DeterministicLevelSerializer.scala

+
+private[protobuf] object DeterministicLevelSerializer {
+
+  def serialize(input: DeterministicLevel.Value): GDeterministicLevel = {


I still want to know which of the following code and case match is faster

private lazy val scalaToPb = Map( DeterministicLevel.DETERMINATE -> GDeterministicLevel.DETERMINISTIC_LEVEL_DETERMINATE, DeterministicLevel.UNORDERED -> GDeterministicLevel.DETERMINISTIC_LEVEL_UNORDERED, DeterministicLevel.INDETERMINATE -> GDeterministicLevel.DETERMINISTIC_LEVEL_INDETERMINATE ) def serialize(input: DeterministicLevel.Value): GDeterministicLevel = scalaToPb(input)

A simple test is as follows:

LuciferYang@3d4dc25

run with GA: https://github.com/LuciferYang/spark/actions/runs/3816561346

case match is slow:

OpenJDK 64-Bit Server VM 1.8.0_352-b08 on Linux 5.15.0-1023-azure Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz Test serialize: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ Use map 192 195 3 5.2 192.4 1.0X Use case match 454 455 1 2.2 453.8 0.4X

@LuciferYang Could you run it again in reverse order？
Because when I run on local as follow, result:

It is very weird!

@LuciferYang When I run it on GA, result as follow:

https://github.com/panbingkun/spark/actions/runs/3816951445/jobs/6492899136
Code:
https://github.com/apache/spark/pull/39286/files#diff-eca16ee278b786f4ab866072396455b93c6bedd8e3b8d562935623931ae3936cR36-R58

OK, I see , it fine to me to use case match now. I think we should investigate why the result data changes with the test order , but this does not block this pr

…atus` to follow with the code style

LuciferYang · 2023-01-01T13:44:54Z

cloud you update the pr title and description

gengliangwang · 2023-01-04T08:02:39Z

core/src/test/scala/org/apache/spark/status/protobuf/SerializerBenchmark.scala

+import org.apache.spark.benchmark.{Benchmark, BenchmarkBase}
+import org.apache.spark.status.protobuf.StoreTypes.{DeterministicLevel => GDeterministicLevel}
+
+object SerializerBenchmark extends BenchmarkBase {


I don't think we need to put the benchmark into the code base.

Let me delete it.

gengliangwang · 2023-01-04T08:03:40Z

core/src/main/scala/org/apache/spark/status/protobuf/RDDOperationGraphWrapperSerializer.scala

  private def serializeRDDOperationNode(node: RDDOperationNode): StoreTypes.RDDOperationNode = {
-    val outputDeterministicLevel = StoreTypes.RDDOperationNode.DeterministicLevel
-      .valueOf(node.outputDeterministicLevel.toString)
+    val outputDeterministicLevel = DeterministicLevelSerializer.serialize(


Since DeterministicLevelSerializer is only used here, shall we move it into this file?

gengliangwang

LGTM except for two minor comments.

…e code style

srowen · 2023-01-04T16:59:00Z

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

-  SUCCEEDED = 2;
-  FAILED = 3;
-  UNKNOWN = 4;
+  JOB_EXECUTION_STATUS_RUNNING = 1;


This feels redundant to have the name in the enum class and in the constant name?

@srowen Yes, but we are following https://developers.google.com/protocol-buffers/docs/style#enums here. The purpose is to avoid naming conflicts. For example, if there is another enum containing FAILED or SUCCEEDED, the Protobuf compiler won't fail.

Oh ok I get it

gengliangwang · 2023-01-04T21:14:21Z

Thanks, merging to master

[SPARK-41768][CORE] Refactor the definition of enum - `JobExecutionSt…

972ff8a

…atus` to follow with the code style

github-actions bot added CORE SQL labels Dec 29, 2022

panbingkun mentioned this pull request Dec 29, 2022

[SPARK-41423][CORE] Protobuf serializer for StageDataWrapper #39192

Closed

Merge branch 'master' into SPARK-41768

a913bd1

LuciferYang reviewed Dec 30, 2022

View reviewed changes

[SPARK-41768][CORE] Refactor the definition of enum - `JobExecutionSt…

bdca5f4

…atus` to follow with the code style

[SPARK-41768][CORE] Refactor the definition of enum - `JobExecutionSt…

8eff720

…atus` to follow with the code style

panbingkun requested a review from gengliangwang January 1, 2023 09:25

LuciferYang reviewed Jan 1, 2023

View reviewed changes

[SPARK-41768][CORE] Refactor the definition of enum - `JobExecutionSt…

0609a69

…atus` to follow with the code style

panbingkun changed the title ~~[SPARK-41768][CORE] Refactor the definition of enum - JobExecutionStatus to follow with the code style~~ [SPARK-41768][CORE] Refactor the definition of enum in protobuf to follow with the code style Jan 1, 2023

panbingkun changed the title ~~[SPARK-41768][CORE] Refactor the definition of enum in protobuf to follow with the code style~~ [SPARK-41768][CORE] Refactor the definition of enum to follow with the code style Jan 2, 2023

gengliangwang reviewed Jan 4, 2023

View reviewed changes

gengliangwang approved these changes Jan 4, 2023

View reviewed changes

[SPARK-41768][CORE] Refactor the definition of enum to follow with th…

92a8545

…e code style

srowen reviewed Jan 4, 2023

View reviewed changes

gengliangwang closed this in 492356d Jan 4, 2023


		private[protobuf] object DeterministicLevelSerializer {

		def serialize(input: DeterministicLevel.Value): GDeterministicLevel = {

Comments

Conversation

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

Uh oh!

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!

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!

panbingkun commented Dec 31, 2022

Uh oh!

AmplabJenkins commented Dec 31, 2022

Uh oh!

gengliangwang commented Jan 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panbingkun Jan 1, 2023 • 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!

panbingkun Jan 1, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Jan 1, 2023

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!

gengliangwang left a comment

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!

gengliangwang commented Jan 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

panbingkun commented Dec 29, 2022 •

edited

Loading

panbingkun Jan 1, 2023 •

edited

Loading

panbingkun Jan 1, 2023 •

edited

Loading