Skip to content

Conversation

@nija-at
Copy link
Contributor

@nija-at nija-at commented Apr 25, 2024

What changes were proposed in this pull request?

When the server closes a session, usually after a cluster restart,
the client is unaware of this until it receives an error.

At this point, the client in unable to create a new session to the
same connect endpoint, since the stale session is still recorded
as the active and default session.

With this change, when the server communicates that the session
has changed via a GRPC error, the session and the respective client
are marked as stale. A new default connection can be created
via the session builder.

Why are the changes needed?

See section above.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Attached unit tests

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

No.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

seems fine from a cursory look ..... but would be great if @juliuszsompolski has a chance to take a quick look ...

@juliuszsompolski
Copy link
Contributor

The SESSION_CHANGED error comes from #45499, cc @nemanja-boric-databricks and @hvanhovell . This seems like a followup-fix to that change and it looks good to me, but it would be best if Nemanja took a look.
One question from me: do we need that fix in scala client as well?

Copy link
Contributor

@nemanja-boric-databricks nemanja-boric-databricks left a comment

Choose a reason for hiding this comment

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

LGTM, yes we most likely need the same for Scala.

@zhengruifeng
Copy link
Contributor

thanks, merged to master

@juliuszsompolski
Copy link
Contributor

yes we most likely need the same for Scala.

@nemanja-boric-databricks @nija-at would one of you have time to followup and check and fix for scala if needed?

@nemanja-boric-databricks
Copy link
Contributor

Yeah, I'll followup with the fix PR for scala client

@ghost
Copy link

ghost commented Jun 18, 2024

@juliuszsompolski I just ported the fix + the follow-up to Scala - #47008, could you review this?

HyukjinKwon pushed a commit that referenced this pull request Jun 19, 2024
… the default session is closed by the server

### What changes were proposed in this pull request?

This is a Scala port of #46221 and #46435.

A client is unaware of a server restart or the server having closed the client until it receives an error. However, at this point, the client in unable to create a new session to the same connect endpoint, since the stale session is still recorded
as the active and default session.

With this change, when the server communicates that the session has changed via a GRPC error, the session and the respective client are marked as stale, thereby allowing a new default connection can be created via the session builder.

In some cases, particularly when running older versions of the Spark cluster (3.5), the error actually manifests as a mismatch in the observed server-side session id between calls. With this fix, we also capture this case and ensure that this case is
also handled.

### Why are the changes needed?

Being unable to use getOrCreate() after an error is unacceptable and should be fixed.

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

No

### How was this patch tested?

./build/sbt testOnly *SparkSessionE2ESuite

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

No

Closes #47008 from changgyoopark-db/SPARK-47986.

Authored-by: Changgyoo Park <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
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.

6 participants