Skip to content

Conversation

@nemanja-boric-databricks
Copy link
Contributor

@nemanja-boric-databricks nemanja-boric-databricks commented Mar 13, 2024

What changes were proposed in this pull request?

In this PR we change the client behaviour to send the previously observed server session id so that the server can validate that the client used to talk with this specific session. Previously this was only validated on the client side which made the server actually execute the request for the wrong session before throwing on the client side (once the response from the server was obtained).

Why are the changes needed?

The server can execute the client command on the wrong spark session before client figuring out it's the different session.

Does this PR introduce any user-facing change?

The error message now pops up differently (it used to be a slightly different message when validated on the client).

How was this patch tested?

Existing unit tests, add new unit test, e2e test added, manual testing

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

No

…n is the same

In this PR we change the client behaviour to send the previously observed
server session id so that the server can validate that the client used
to talk with this specific session. Previously this was only validated
on the client side which made the server actually execute the request
for the wrong session before throwing on the client side (once the response
from the server was obtained).

Testing:
Existing unit tests, add new unit test, e2e test added, manual testing
@nemanja-boric-databricks
Copy link
Contributor Author

nemanja-boric-databricks commented Mar 14, 2024

Failed suite is:

[info] *** 1 SUITE ABORTED ***
[error] Error during tests:
[error] 	org.apache.spark.sql.jdbc.v2.OracleIntegrationSuite

But it passed for the previous commits in this PR, it seems unrelated.

.setClientType(userAgent)
.build()
bstub.releaseSession(request)
bstub.releaseSession(request.build())
Copy link
Contributor

Choose a reason for hiding this comment

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

These two lines are equivalent right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is just to make it uniform to everything else.

Copy link
Contributor

@hvanhovell hvanhovell left a comment

Choose a reason for hiding this comment

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

LGTM

@hvanhovell
Copy link
Contributor

Merging test failure is unrelated.

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.

2 participants