You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "nija-at (via GitHub)" <gi...@apache.org> on 2023/12/05 09:37:03 UTC
[PR] [SPARK-46265] Assertions in AddArtifact RPC make the connect client incompatible with older clusters [spark]
nija-at opened a new pull request, #44181:
URL: https://github.com/apache/spark/pull/44181
### What changes were proposed in this pull request?
A previous commit - d9c5f9d6 - updated the response of the
`AddArtifact` RPC to return the session id. Further, it also added
an assertion to the client of this RPC asserting that the session id
returned by the server matches the session id that was requested.
However, we will have the case that a connect client with this new
assertion included may connect to a Spark cluster that does not yet
have this change, making the change backwards incompatible.
Loosen the assertion to allow empty session id in the RPC's response.
### Why are the changes needed?
Newer connect clients can connect with older Spark clusters.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Manually started the a connect cluster using `spark-connect-shell`
and using the connect client shell via `spark-connect-scala-client`.
Changed the service to not return session id and verified the fix.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46265][CONNECT] Assertions in AddArtifact RPC make the connect client incompatible with older clusters [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell closed pull request #44181: [SPARK-46265][CONNECT] Assertions in AddArtifact RPC make the connect client incompatible with older clusters
URL: https://github.com/apache/spark/pull/44181
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46265][CONNECT] Assertions in AddArtifact RPC make the connect client incompatible with older clusters [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #44181:
URL: https://github.com/apache/spark/pull/44181#discussion_r1415358128
##########
connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ArtifactManager.scala:
##########
@@ -125,7 +125,7 @@ class ArtifactManager(
.addAllNames(Arrays.asList(artifactName))
.build()
val response = bstub.artifactStatus(request)
- if (response.getSessionId != sessionId) {
+ if (StringUtils.isNotEmpty(response.getSessionId) && response.getSessionId != sessionId) {
Review Comment:
Maybe add a comment that explains why an empty session ID is treated differently. Here and below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46265][CONNECT] Assertions in AddArtifact RPC make the connect client incompatible with older clusters [spark]
Posted by "nija-at (via GitHub)" <gi...@apache.org>.
nija-at commented on code in PR #44181:
URL: https://github.com/apache/spark/pull/44181#discussion_r1415528585
##########
connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ArtifactManager.scala:
##########
@@ -125,7 +125,7 @@ class ArtifactManager(
.addAllNames(Arrays.asList(artifactName))
.build()
val response = bstub.artifactStatus(request)
- if (response.getSessionId != sessionId) {
+ if (StringUtils.isNotEmpty(response.getSessionId) && response.getSessionId != sessionId) {
Review Comment:
added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org