You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "vicennial (via GitHub)" <gi...@apache.org> on 2023/02/23 21:31:07 UTC
[GitHub] [spark] vicennial opened a new pull request, #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
vicennial opened a new pull request, #40147:
URL: https://github.com/apache/spark/pull/40147
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR introduces a new client-streaming RPC service "AddArtifacts" to handle the transfer of artifacts from the client to the server. New message types `AddArtifactsRequest` and `AddArtifactsResponse` are added that specify the format of artifact transfer.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
In the decoupled client-server architecture of Spark Connect, a remote client may use a local JAR or a new class in their UDF that may not be present on the server. To handle these cases of missing "artifacts", a protocol for artifact transfer is needed to move the required artifacts from the client side over to the server side.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
N/A
--
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
[GitHub] [spark] xkrogen commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1119355670
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,87 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ // CRC to allow server to verify integrity of the artifact.
+ int64 crc = 3;
Review Comment:
I like the general framework, yeah. I like `CompactArtifact` or `SingleChunkArtifact` better than `SingleArtifact`. Or maybe `CompleteArtifact`, referring to the fact that it is "complete" i.e. doesn't require any future chunks.
For chunked artifact, I like it with "begin" or "start" better to make it clear that there is more to follow (i.e. it's not self-contained)
--
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
[GitHub] [spark] vicennial commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1117219600
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
+ // A large artifact chunked into multiple requests. The server side should assume that the
+ // artifact has been completely uploaded either when it encounters a new artifact name, or
+ // when the the stream is completed.
+ Artifact chunk = 4;
+ }
+}
+
+// Response to adding an artifact. Contains relevant metadata to verify successful transfer of
+// artifact(s).
+message AddArtifactsResponse {
+ // Metadata of an artifact.
+ message ArtifactSummary {
+ string name = 1;
+ // Size in bytes.
+ int64 size = 2;
+ // CRC to verify integrity of the transferred artifact.
+ int64 crc = 3;
Review Comment:
> Usually the sender of the artifact (the client) would provide the crc, so that the receiver (the server) can validate it after receiving.
True, I've updated it to follow the typical convention and move crc validation to the server side. An extra benefit of this is that we can drop the incorrect data on server side if crc fails. Thanks for the suggestion!
> How is the client supposed to respond if the crc is incorrect?
The client may choose to reupload the artifact if the CRC is incorrect. I have added this to the documentation to clarify.
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
+ // A large artifact chunked into multiple requests. The server side should assume that the
+ // artifact has been completely uploaded either when it encounters a new artifact name, or
+ // when the the stream is completed.
+ Artifact chunk = 4;
+ }
+}
+
+// Response to adding an artifact. Contains relevant metadata to verify successful transfer of
+// artifact(s).
+message AddArtifactsResponse {
+ // Metadata of an artifact.
+ message ArtifactSummary {
+ string name = 1;
+ // Size in bytes.
+ int64 size = 2;
+ // CRC to verify integrity of the transferred artifact.
+ int64 crc = 3;
Review Comment:
> Usually the sender of the artifact (the client) would provide the crc, so that the receiver (the server) can validate it after receiving.
True, I've updated it to follow the typical convention and moved crc validation to the server side. An extra benefit of this is that we can drop the incorrect data on server side if crc fails. Thanks for the suggestion!
> How is the client supposed to respond if the crc is incorrect?
The client may choose to reupload the artifact if the CRC is incorrect. I have added this to the documentation to clarify.
--
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
[GitHub] [spark] hvanhovell closed pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell closed pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
URL: https://github.com/apache/spark/pull/40147
--
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
[GitHub] [spark] hvanhovell commented on pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #40147:
URL: https://github.com/apache/spark/pull/40147#issuecomment-1442630247
@xkrogen yes, Spark Connect decouples client and server. That we synchronize jars and REPL generated classes does not mean we are re-introducing the coupling between client and server. All it means is that we will be using the server (driver) to orchistrate the execution of user defined code. This code could be executed within the same engine, however we can also use separate processes or VMs to execute this code. I do feel that adding this kind of functionality to connect keeps thing simple from the client's POV; it does not make a lot sense to me to do the synchronisation through a different service, because we need to go through the driver anyway.
I do want to call out that this mechanism is just not here for JARs and REPL generated classed, but that there are also quite a few other use cases that we need this mechanism for, e.g.: reading client local files, synchronizing other kinds of dependencies (for example python wheels), uploading models, ...
Finally it is early days for connect. We currently want to make it easy to folks to try and to move to connect. We need a REPL experience that is similar to the current experience for that. We want to do the simple thing first, and that is reuse the current UDF execution code (with some classpath isolation). I am happy to discuss the steps after that, if you are up to that please reach out to me (herman@databricks.com).
--
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
[GitHub] [spark] xkrogen commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1117343251
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -193,5 +247,9 @@ service SparkConnectService {
// Analyzes a query and returns a [[AnalyzeResponse]] containing metadata about the query.
rpc AnalyzePlan(AnalyzePlanRequest) returns (AnalyzePlanResponse) {}
+
+ // Add artifacts to the session and returns a [[AddArtifactsResponse]] containing metadata about
+ // the added artifacts.
+ rpc AddArtifacts(stream AddArtifactsRequest) returns (AddArtifactsResponse) {}
Review Comment:
Cool. Some benchmarking would still be nice at some point, even on JVM, but that's good to hear.
--
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
[GitHub] [spark] xkrogen commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1117330163
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
Review Comment:
Good point; you have definitely convinced me that supporting the raw data xfer as specified here is needed. I do think we should consider Ivy coordinate support as a future extension, at least, and make sure we can support it here. Right now `bytes data` is a required field, so we may need to adjust that to support future ways of specifying the required content?
--
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
[GitHub] [spark] vicennial commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1117224826
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
+ // A large artifact chunked into multiple requests. The server side should assume that the
+ // artifact has been completely uploaded either when it encounters a new artifact name, or
+ // when the the stream is completed.
+ Artifact chunk = 4;
+ }
+}
+
+// Response to adding an artifact. Contains relevant metadata to verify successful transfer of
+// artifact(s).
+message AddArtifactsResponse {
+ // Metadata of an artifact.
+ message ArtifactSummary {
+ string name = 1;
+ // Size in bytes.
+ int64 size = 2;
Review Comment:
The server sends back a single message for the stream of client inputs so the `name` is needed to link the `crc` to the artefact.
The `crc` field is now replaced by the `is_crc_successful` (server-side validation) and I've dropped the `size` field (it is indeed redundant since the client will know its size)
--
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
[GitHub] [spark] xkrogen commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1117328132
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
Review Comment:
Agreed that we need the ability to both chunk and to batch, but my point is that `repeated Artifact artifacts` already achieves this by allowing you to put one artifact (for chunking purposes) or many (for small classes). So instead of:
```proto
message Batch {
repeated Artifact artifacts = 1;
}
oneof payload {
Batch batch = 3;
Artifact chunk = 4;
}
```
you would just have:
```
repeated Artifact artifacts = 3;
```
AFAICT there is no loss of functionality here? You can choose to send an `AddArtifactsRequest` with just a single artifact, same as if you used the `Artifact chunk` branch of `payload`. Or you can choose to send a request with multiple artifacts, same as if you used the `Batch`. It's just that you don't need to wrap those artifacts inside of a `Batch`.
--
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
[GitHub] [spark] vicennial commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1117931152
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,87 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ // CRC to allow server to verify integrity of the artifact.
+ int64 crc = 3;
Review Comment:
Yes, that makes sense but I'm wondering how we should proceed with the naming since the naming `Aritifact` should technically represent any artifact regardless of size.
A proposal for the naming:
Name the smallest unit of data as `ArtifactChunk`
```
message ArtifactChunk {
bytes data = 1;
int64 crc = 2;
}
```
Use the name `SingleAritifact` (or perhaps `CompactArtifact`) instead of `Artifact`
```
message SingleArtifact {
string name = 1;
ArtifactChunk data = 2;
}
```
Rename `BeginChunkedArtifact` to `ChunkedArtifact`
```
message ChunkedArtifact {
string name = 1;
int64 total_bytes = 2;
int64 num_chunks = 3;
ArtifactChunk initial_chunk = 4;
}
```
Now we can define an "Artifact" (verbally) as either a {`SingleArtifact`} or {`ChunkedAritfact` + it's appended `ArtifactChunks`}. WDYT?
--
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
[GitHub] [spark] vicennial commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1117927851
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
+ // A large artifact chunked into multiple requests. The server side should assume that the
+ // artifact has been completely uploaded either when it encounters a new artifact name, or
+ // when the the stream is completed.
+ Artifact chunk = 4;
Review Comment:
> Do we also need EndChunkedArtifact?
The `BeginChunkedArtifact` contains some metadata but I'm not sure what other data we can include in `EndChunkedArtifact` that's also not available in `BeginChunkedArtifact` so I've left it out in order to keep message types minimal. However, no strong preference for either way.
--
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
[GitHub] [spark] vicennial commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1120455152
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
Review Comment:
The introduction of `SingleChunkArtifact`, `BeginChunkedArtifact` etc helps with extensibility because we can now add a new message type, say "IvyArtifact", and include it in the `payload` of `AddArtifactsRequest`.
--
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
[GitHub] [spark] xkrogen commented on pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on PR #40147:
URL: https://github.com/apache/spark/pull/40147#issuecomment-1451013462
thanks for incorporating the feedback!
--
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
[GitHub] [spark] hvanhovell commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1116398414
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -193,5 +247,9 @@ service SparkConnectService {
// Analyzes a query and returns a [[AnalyzeResponse]] containing metadata about the query.
rpc AnalyzePlan(AnalyzePlanRequest) returns (AnalyzePlanResponse) {}
+
+ // Add artifacts to the session and returns a [[AddArtifactsResponse]] containing metadata about
+ // the added artifacts.
+ rpc AddArtifacts(stream AddArtifactsRequest) returns (AddArtifactsResponse) {}
Review Comment:
AFAIK that is only problem when you stream data to a python process, and not the other way around.
--
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
[GitHub] [spark] hvanhovell commented on pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #40147:
URL: https://github.com/apache/spark/pull/40147#issuecomment-1449127907
I am merging this. Let's address further concerns in a follow-up.
--
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
[GitHub] [spark] xkrogen commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1116278139
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
+ // A large artifact chunked into multiple requests. The server side should assume that the
+ // artifact has been completely uploaded either when it encounters a new artifact name, or
+ // when the the stream is completed.
+ Artifact chunk = 4;
+ }
+}
+
+// Response to adding an artifact. Contains relevant metadata to verify successful transfer of
+// artifact(s).
+message AddArtifactsResponse {
+ // Metadata of an artifact.
+ message ArtifactSummary {
+ string name = 1;
+ // Size in bytes.
+ int64 size = 2;
+ // CRC to verify integrity of the transferred artifact.
+ int64 crc = 3;
Review Comment:
Usually the sender of the artifact (the client) would provide the crc, so that the receiver (the server) can validate it after receiving. Why do we return the crc from the server back to the client? How is the client supposed to respond if the crc is incorrect?
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
+ // A large artifact chunked into multiple requests. The server side should assume that the
+ // artifact has been completely uploaded either when it encounters a new artifact name, or
+ // when the the stream is completed.
+ Artifact chunk = 4;
Review Comment:
This protocol basically defines the end of an artifact by the start of a new artifact. It feels awkward. Why not include some metadata, such as total expected length for an artifact, to make it easy for the server to determine up front whether it is going to receive a full artifact or just a chunk of it? This could also make it easier for the server-side to, for example, allocate the entire required space up front.
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -193,5 +247,9 @@ service SparkConnectService {
// Analyzes a query and returns a [[AnalyzeResponse]] containing metadata about the query.
rpc AnalyzePlan(AnalyzePlanRequest) returns (AnalyzePlanResponse) {}
+
+ // Add artifacts to the session and returns a [[AddArtifactsResponse]] containing metadata about
+ // the added artifacts.
+ rpc AddArtifacts(stream AddArtifactsRequest) returns (AddArtifactsResponse) {}
Review Comment:
There's some somewhat concerning language about stream performance in Python on gRPC's [performance page](https://grpc.io/docs/guides/performance/), can you provide any benchmarks about how well this performs for JVM/Python clients? Individual JAR files, and the number of JAR files needed, can become quite large.
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
Review Comment:
This seems unnecessarily complex. Couldn't we just have `repeated Artifact artifacts` here directly? The client can choose to put multiple, or just one. What value is the `Batch` abstraction providing? Why is there an important distinction between a `Batch` and a single artifact?
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
Review Comment:
Should consider allowing clients to specify artifacts from Ivy coordinates, either in addition or instead of directly providing the contents of a JAR? This seems to be a cleaner separation of client and server; the client shouldn't need to have a copy of the JARs it will use to execute some UDF. Spark already has support for localizing artifacts from an external repository.
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
+ // A large artifact chunked into multiple requests. The server side should assume that the
+ // artifact has been completely uploaded either when it encounters a new artifact name, or
+ // when the the stream is completed.
+ Artifact chunk = 4;
+ }
+}
+
+// Response to adding an artifact. Contains relevant metadata to verify successful transfer of
+// artifact(s).
+message AddArtifactsResponse {
+ // Metadata of an artifact.
+ message ArtifactSummary {
+ string name = 1;
+ // Size in bytes.
+ int64 size = 2;
Review Comment:
doesn't the client already know both of these? why do we need to send it back?
--
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
[GitHub] [spark] xkrogen commented on pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on PR #40147:
URL: https://github.com/apache/spark/pull/40147#issuecomment-1444053186
> This code could be executed within the same engine, however we can also use separate processes or VMs to execute this code.
Good point; nothing about the protocol itself precludes us from using separate UDF processes in the future.
> Finally it is early days for connect. We currently want to make it easy to folks to try and to move to connect ...
Generally agreed, but APIs and contracts can be challenging to change once adopted, so it's important to get it right on the first try, at least to the extent that it doesn't force us into bad decisions or tech debt later. That's why I'm interested in discussing a longer-term roadmap about this kind of support.
But I think you have convinced me that this protocol is generally useful!
cc @mridulm in case you're interested in taking a look
--
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
[GitHub] [spark] vicennial commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1119983837
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,87 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ // CRC to allow server to verify integrity of the artifact.
+ int64 crc = 3;
Review Comment:
👍 I've used `SingleChunkArtifact` to avoid ambiguity with the meaning of "compact" or "complete" (multi-chunk case)
--
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
[GitHub] [spark] vicennial commented on pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on PR #40147:
URL: https://github.com/apache/spark/pull/40147#issuecomment-1448558937
@xkrogen Thank you for all of the feedback! Do you mind doing a review of the current state? We're looking to merge and were wondering if there are any other points to address.
--
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
[GitHub] [spark] hvanhovell commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1116397268
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
Review Comment:
There is a bit of a funny problem here. On the one hand you have jars, they tend to be large and you want to upload them in one or more chunks. On the other hand there are REPL generated classes, these are tiny but there can be many, in this case it makes sense to batch them to reduce RPC overhead.
--
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
[GitHub] [spark] hvanhovell commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1116395939
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
Review Comment:
We can add it. Ivy coordinates become a bit of a hassle when someone is using a private repository. Quite a few of our customers provides jars as is, they don't want to or can't manage repositories for various reasons. Furthermore we also need to deal with REPL generated code, and there are a bunch of other use cases that still need this kind of synchronization.
--
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
[GitHub] [spark] xkrogen commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1117339081
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,87 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ // CRC to allow server to verify integrity of the artifact.
+ int64 crc = 3;
Review Comment:
should we replace these with `ArtifactChunk`? As defined currently, an `Artifact` is basically just a single-chunk entity associated with a name.
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
+ // A large artifact chunked into multiple requests. The server side should assume that the
+ // artifact has been completely uploaded either when it encounters a new artifact name, or
+ // when the the stream is completed.
+ Artifact chunk = 4;
Review Comment:
This makes sense to me. Do we also need `EndChunkedArtifact`? The server-side can infer it based on `num_chunks`, but I wonder if being explicit is better?
--
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
[GitHub] [spark] vicennial commented on a diff in pull request #40147: [SPARK-42543][CONNECT] Specify protocol for UDF artifact transfer in JVM/Scala client
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on code in PR #40147:
URL: https://github.com/apache/spark/pull/40147#discussion_r1117229154
##########
connector/connect/common/src/main/protobuf/spark/connect/base.proto:
##########
@@ -183,6 +183,60 @@ message ExecutePlanResponse {
}
}
+// Request to transfer client-local artifacts.
+message AddArtifactsRequest {
+
+ // Definition of an Artifact.
+ message Artifact {
+ // The name of the artifact is expected in the form of a "Relative Path" that is made up of a
+ // sequence of directories and the final file element.
+ // Examples of "Relative Path"s: "jars/test.jar", "classes/xyz.class", "abc.xyz", "a/b/X.jar".
+ // The server is expected to maintain the hierarchy of files as defined by their name. (i.e
+ // The relative path of the file on the server's filesystem will be the same as the name of
+ // the provided artifact)
+ string name = 1;
+ // Raw data.
+ bytes data = 2;
+ }
+
+ // A number of small artifacts batched into a single RPC.
+ message Batch {
+ repeated Artifact artifacts = 1;
+ }
+
+ // The client_id is set by the client to be able to collate streaming responses from
+ // different queries.
+ string client_id = 1;
+
+ // User context
+ UserContext user_context = 2;
+
+ // The payload is either a batch of artifacts or a partial chunk of a large artifact.
+ oneof payload {
+ Batch batch = 3;
+ // A large artifact chunked into multiple requests. The server side should assume that the
+ // artifact has been completely uploaded either when it encounters a new artifact name, or
+ // when the the stream is completed.
+ Artifact chunk = 4;
Review Comment:
Thanks for the feedback.
I've introduced two new message types to handle this:
The `BeginChunkedArtifact` message now contains the metadata of the chunked artifact as well as the initial chunk. Messages following this would be of `ArtifactChunk` type which represents the difference between an artifact chunk and a "batch" of artifact (which may either be a single, complete artifact or multiple complete artifacts).
--
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