Skip to content

Comments

[SPARK-41754][UI] Add simple developer guides for UI Protobuf serializer#39270

Closed
gengliangwang wants to merge 2 commits intoapache:masterfrom
gengliangwang:addGuideline
Closed

[SPARK-41754][UI] Add simple developer guides for UI Protobuf serializer#39270
gengliangwang wants to merge 2 commits intoapache:masterfrom
gengliangwang:addGuideline

Conversation

@gengliangwang
Copy link
Member

What changes were proposed in this pull request?

  • Add simple developer guides for UI Protobuf serializer.
  • Rename UNSPECIFIED in the enum JobExecutionStatus as JOB_EXECUTION_STATUS_UNSPECIFIED

Why are the changes needed?

  1. Provide a simple guide for common issues found during PR review
  2. Avoid naming conflicts in enum definitions.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing UT

@github-actions github-actions bot added the CORE label Dec 29, 2022
@gengliangwang
Copy link
Member Author

This is for the issue in #39192 (comment)
cc @LuciferYang @panbingkun

* Developer guides:
* - Coding style: https://developers.google.com/protocol-buffers/docs/style
* - Use int64 for job/stage IDs, in case of future extension in Spark core.
* - Use `weakIntern` on string values in create new objects during deserialization.
Copy link
Contributor

@LuciferYang LuciferYang Dec 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Use weakIntern on string values in create new objects during deserialization.

This is very important. I think we need a refactor work to fix the current codes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@panbingkun panbingkun Dec 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@LuciferYang LuciferYang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM

@LuciferYang
Copy link
Contributor

LuciferYang commented Dec 29, 2022

need re-trigger failed GA task @gengliangwang


enum JobExecutionStatus {
UNSPECIFIED = 0;
JOB_EXECUTION_STATUS_UNSPECIFIED = 0;
Copy link
Contributor

@panbingkun panbingkun Dec 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it more reasonable?

message Job {
   enum JobExecutionStatus {
      ...
   }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gengliangwang
Copy link
Member Author

Merging to master

gengliangwang pushed a commit that referenced this pull request Jan 19, 2023
…bjects during deserialization

### What changes were proposed in this pull request?
The pr aims to use weakIntern on string values in create new objects during deserialization.

### Why are the changes needed?
Following guid: #39270.
<img width="587" alt="image" src="https://user-images.githubusercontent.com/15246973/209924939-779db183-f0a6-4f3b-aebf-cf00b6c95cf8.png">

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

### How was this patch tested?
Pass GA.

Closes #39275 from panbingkun/SPARK-41759.

Authored-by: panbingkun <pbk1982@gmail.com>
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants