Skip to content

Conversation

@pan3793
Copy link
Member

@pan3793 pan3793 commented Sep 22, 2024

What changes were proposed in this pull request?

SparkListenerConnectServiceStarted is introduced in SPARK-47952, while the referenced field SparkConf is not serialized properly, then causes the SHS deserialization failure.

According to the discussion, we can remove the sparkConf field.

Why are the changes needed?

Fix the event log deserialization and recover the SHS UI rendering.

Does this PR introduce any user-facing change?

Fix an unreleased feature, recover the SHS UI rendering from event logs produced by the connect server.

How was this patch tested?

Start a connect server with event log enabled, and then open the UI in SHS.

4.0.0-preview2
image

This PR.
image

Was this patch authored or co-authored using generative AI tooling?

No.

@pan3793
Copy link
Member Author

pan3793 commented Sep 22, 2024

hostAddress: String,
bindingPort: Int,
sparkConf: SparkConf,
sparkConf: Map[String, String],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can anyone explain why we are sending all spark confs? That seems to be a bit heavyweight.

Copy link
Member Author

Choose a reason for hiding this comment

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

regarding use case mentioned in SPARK-47952, I think sparkConf is not necessary, maybe the author can answer this question @TakawaAkirayo

Copy link
Contributor

@TakawaAkirayo TakawaAkirayo Sep 25, 2024

Choose a reason for hiding this comment

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

The purpose is to send additional context, such as the spark.app.id set in SparkContext's _conf during startup, to the listener. This can be achieved through other means within the listener and this seems over-designed now, let's remove it @pan3793

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, updated and verified again, please take another look, @hvanhovell and @TakawaAkirayo

Copy link
Contributor

@TakawaAkirayo TakawaAkirayo left a comment

Choose a reason for hiding this comment

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

LGTM

@pan3793
Copy link
Member Author

pan3793 commented Sep 29, 2024

kindly ping @hvanhovell

@HyukjinKwon
Copy link
Member

Merged to master.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants