You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2023/01/29 08:41:04 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #39789: [SPARK-42228][CONNECT][BUILD] Add shade and relocation rule of grpc to connect-client-jvm module
LuciferYang opened a new pull request, #39789:
URL: https://github.com/apache/spark/pull/39789
<!--
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.
-->
### 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.
-->
### 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'.
-->
### 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.
-->
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092862409
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -132,6 +140,10 @@
</includes>
</relocation>
</relocations>
+ <!--SPARK-42228: Add `ServicesResourceTransformer` to relocation class names in META-INF/services for grpc-->
Review Comment:
**Before**
```
ls META-INF/services
io.grpc.LoadBalancerProvider io.grpc.ManagedChannelProvider io.grpc.NameResolverProvider io.grpc.ServerProvider
cat META-INF/services/*
io.grpc.internal.PickFirstLoadBalancerProvider
io.grpc.util.SecretRoundRobinLoadBalancerProvider$Provider
io.grpc.netty.NettyChannelProvider
io.grpc.netty.UdsNettyChannelProvider
io.grpc.netty.UdsNameResolverProvider
io.grpc.netty.NettyServerProvider
```
**After**
```
ls META-INF/services/
org.sparkproject.connect.client.grpc.LoadBalancerProvider org.sparkproject.connect.client.grpc.NameResolverProvider
org.sparkproject.connect.client.grpc.ManagedChannelProvider org.sparkproject.connect.client.grpc.ServerProvider
cat META-INF/services/*
org.sparkproject.connect.client.grpc.internal.PickFirstLoadBalancerProvider
org.sparkproject.connect.client.grpc.util.SecretRoundRobinLoadBalancerProvider$Provider
org.sparkproject.connect.client.grpc.protobuf.services.internal.HealthCheckingRoundRobinLoadBalancerProvider
org.sparkproject.connect.client.grpc.netty.NettyChannelProvider
org.sparkproject.connect.client.grpc.netty.UdsNettyChannelProvider
org.sparkproject.connect.client.grpc.netty.UdsNameResolverProvider
org.sparkproject.connect.client.grpc.internal.DnsNameResolverProvider
org.sparkproject.connect.client.grpc.netty.NettyServerProvider
```
--
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] LuciferYang commented on pull request #39789: [SPARK-42228][CONNECT][BUILD] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1407603498
cc @HyukjinKwon the test demo like https://github.com/apache/spark/pull/39788
We can use the following command to do a comparison test (shaded grpc or not in `connect-client-jvm` module) with the demo change:
```
mvn clean install -pl connector/connect/client/integration-tests -am -Dtest=none -DwildcardSuites=org.apache.spark.sql.ClientE2ETestSuite
```
without shaded grpc with fail.
`ClientE2ETestSuite ` will not cause test failure in the `connect-client-jvm` module because the test is not using the shaded `connect-client-jvm` jar , and the relocation not be execute in that scene.
--
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] zhenlineo commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "zhenlineo (via GitHub)" <gi...@apache.org>.
zhenlineo commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1411143071
>> The reason to use the server shaded jar is because that's the only way to start spark with spark connect.
> Why is this the only way? Because the conflict package name of sql module and connect-client-jvm module?
@LuciferYang Some grpc dependency is not set correctly if we use the spark connect server jar without shading.
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092870964
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -105,11 +105,19 @@
<artifactSet>
<includes>
<include>com.google.guava:*</include>
+ <include>io.grpc:*</include>
<include>com.google.protobuf:*</include>
<include>org.apache.spark:spark-connect-common_${scala.binary.version}</include>
</includes>
</artifactSet>
<relocations>
+ <relocation>
Review Comment:
After
```
ls org/sparkproject/connect/client
grpc guava
ls org/sparkproject/connect/client/grpc
Attributes$1.class LoadBalancer$Helper.class
Attributes$Builder.class LoadBalancer$PickResult.class
Attributes$Key.class LoadBalancer$PickSubchannelArgs.class
Attributes.class LoadBalancer$ResolvedAddresses$Builder.class
BinaryLog.class LoadBalancer$ResolvedAddresses.class
BindableService.class LoadBalancer$Subchannel.class
CallCredentials$MetadataApplier.class LoadBalancer$SubchannelPicker.class
CallCredentials$RequestInfo.class LoadBalancer$SubchannelStateListener.class
CallCredentials.class LoadBalancer.class
CallOptions$Key.class LoadBalancerProvider$UnknownConfig.class
....
```
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092788710
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -105,11 +105,16 @@
<artifactSet>
<includes>
<include>com.google.guava:*</include>
+ <include>io.grpc:*:</include>
Review Comment:
More, I checked the packaging results of the `connect/server` module, It is no longer correct, many packages have not been relocated to `org.sparkproject` package.
I reset the code to [[SPARK-40677][CONNECT][FOLLOWUP] Refactor shade `relocation/rename` rules](https://github.com/apache/spark/pull/38162) and re-checked the packaging results of the `connect/server` module, It's right on that snapshot, all shaded packages are relocated to `org.sparkproject` package.
I'm not sure what changed the packaging results of `server` module, but I think we need to investigate and fix the maven shaded configuration of the `server` modules to ensure that all the shaded packages are relocated to `org.sparkproject` package, I will create a new Jira to tracking this.
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092787895
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -105,11 +105,16 @@
<artifactSet>
<includes>
<include>com.google.guava:*</include>
+ <include>io.grpc:*:</include>
Review Comment:
@zhenlineo double check this change, it only shaded grpc but not relocation.
--
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] zhenlineo commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "zhenlineo (via GitHub)" <gi...@apache.org>.
zhenlineo commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1411215894
@LuciferYang I checked with the maven jar, and I found the io.grpc is still not shaded? And then the guava is shaded twice.
Can you double check?
```
jar -tf <path/to/client.jar>
```
To fix the maven jar, I need the following changes on top of your PR:
```
--- a/connector/connect/client/jvm/pom.xml
+++ b/connector/connect/client/jvm/pom.xml
@@ -105,7 +105,7 @@
<artifactSet>
<includes>
<include>com.google.guava:*</include>
- <include>io.grpc:*:</include>
+ <include>io.grpc:*</include>
<include>com.google.protobuf:*</include>
<include>org.apache.spark:spark-connect-common_${scala.binary.version}</include>
</includes>
@@ -114,6 +114,9 @@
<relocation>
<pattern>io.grpc</pattern>
<shadedPattern>${spark.shade.packageName}.connect.grpc</shadedPattern>
+ <includes>
+ <include>io.grpc.**</include>
+ </includes>
</relocation>
<relocation>
<pattern>com.google.protobuf</pattern>
@@ -124,14 +127,14 @@
</relocation>
<relocation>
<pattern>com.google.common</pattern>
- <shadedPattern>${spark.shade.packageName}.connect.client.guava</shadedPattern>
+ <shadedPattern>${spark.shade.packageName}.connect.guava</shadedPattern>
<includes>
<include>com.google.common.**</include>
</includes>
</relocation>
<relocation>
<pattern>com.google.thirdparty</pattern>
- <shadedPattern>${spark.shade.packageName}.connect.client.guava</shadedPattern>
+ <shadedPattern>${spark.shade.packageName}.connect.guava</shadedPattern>
<includes>
<include>com.google.thirdparty.**</include>
</includes>
```
--
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 #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell closed pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
URL: https://github.com/apache/spark/pull/39789
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092646727
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -105,11 +105,16 @@
<artifactSet>
<includes>
<include>com.google.guava:*</include>
+ <include>io.grpc:*:</include>
Review Comment:
But let me test your suggestion first
--
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] LuciferYang commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1408223480
> Further, I think we should move `ClientE2ETestSuite ` to an independent module, it should not be in `connect-client-jvm` module, because currently only test shaded `connect-server` but not test shaded `connect-client`.
>
> Also cc @zhenlineo
Think again, because of the behavior difference between maven shade and sbt assembly, even if we move e2e test out of `connect-client-jvm` and put it into an independent module in Spark, we can only use maven to test the shaded `connect-client-jvm`.jar, sbt should always fetch the non-shaded `connect-client-jvm`.jar with current dependency configuration way ... am I right?
--
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] zhenlineo commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "zhenlineo (via GitHub)" <gi...@apache.org>.
zhenlineo commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1409593261
Hi @LuciferYang Thanks for fixing this error.
Regarding the E2E test, I want to clarify the purpose of E2E: It is used by scala client to verify client methods are returning the correct results. Due to the nature of the client development, we can hardly verify a lot behaviors without a real server, thus the E2E tests contains a lot of very basic tests and need to run daily. It was not to testing the server shading or client shading. The reason to use the server shaded jar is because that's the only way to start spark with spark connect.
I am not super favor the idea to use E2E also for client/server shading test. Can I suggest we have a separate test to test the client/server shading, following the idea that each test only test one thing?
Really great thanks to help to look into better E2E tests. I also tried to put the E2E in a separate module, I did not get too much benefits but created an empty module that only contains tests. It would be really great if you could work out a way to run the tests as if they are unit tests. Thanks again.
--
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] dongjoon-hyun commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1409076499
Also, cc @sunchao
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092643649
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -105,11 +105,16 @@
<artifactSet>
<includes>
<include>com.google.guava:*</include>
+ <include>io.grpc:*:</include>
Review Comment:
@zhenlineo When I run
`mvn clean install -DskipTests -pl connector/connect/client/jvm`
I can see the shaded logs of grpc, and the server side also writes the same
<img width="857" alt="image" src="https://user-images.githubusercontent.com/1475305/215918872-0d5ad3f1-da33-4851-8f74-78e153caacc0.png">
https://github.com/apache/spark/blob/4d37e7816ce83dc5928b0d42a536304cb49eb0a3/connector/connect/server/pom.xml#L244
https://github.com/apache/spark/blob/4d37e7816ce83dc5928b0d42a536304cb49eb0a3/connector/connect/server/pom.xml#L284-L287
--
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] LuciferYang commented on pull request #39789: [SPARK-42228][CONNECT][BUILD] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1407605388
Further, I think we should move `ClientE2ETestSuite ` to an independent module, it should not be in `connect-client-jvm` module, because currently only test shaded `connect-server` but not test shaded `connect-client`.
--
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] HyukjinKwon commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1408019147
cc @grundprinzip too
--
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] LuciferYang commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1411259857
> @LuciferYang I checked with the maven jar, and I found the io.grpc is still not shaded? And then the guava is shaded twice. Can you double check?
>
> ```
> jar -tf <path/to/client.jar>
> ```
>
> To fix the maven jar, I need the following changes on top of your PR:
>
> ```
> --- a/connector/connect/client/jvm/pom.xml
> +++ b/connector/connect/client/jvm/pom.xml
> @@ -105,7 +105,7 @@
> <artifactSet>
> <includes>
> <include>com.google.guava:*</include>
> - <include>io.grpc:*:</include>
> + <include>io.grpc:*</include>
> <include>com.google.protobuf:*</include>
> <include>org.apache.spark:spark-connect-common_${scala.binary.version}</include>
> </includes>
> @@ -114,6 +114,9 @@
> <relocation>
> <pattern>io.grpc</pattern>
> <shadedPattern>${spark.shade.packageName}.connect.grpc</shadedPattern>
> + <includes>
> + <include>io.grpc.**</include>
> + </includes>
> </relocation>
> <relocation>
> <pattern>com.google.protobuf</pattern>
> @@ -124,14 +127,14 @@
> </relocation>
> <relocation>
> <pattern>com.google.common</pattern>
> - <shadedPattern>${spark.shade.packageName}.connect.client.guava</shadedPattern>
> + <shadedPattern>${spark.shade.packageName}.connect.guava</shadedPattern>
> <includes>
> <include>com.google.common.**</include>
> </includes>
> </relocation>
> <relocation>
> <pattern>com.google.thirdparty</pattern>
> - <shadedPattern>${spark.shade.packageName}.connect.client.guava</shadedPattern>
> + <shadedPattern>${spark.shade.packageName}.connect.guava</shadedPattern>
> <includes>
> <include>com.google.thirdparty.**</include>
> </includes>
> ```
Thanks for your review @zhenlineo , let me double check this today
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092862409
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -132,6 +140,10 @@
</includes>
</relocation>
</relocations>
+ <!--SPARK-42228: Add `ServicesResourceTransformer` to relocation class names in META-INF/services for grpc-->
Review Comment:
**Before**
```
ls META-INF/services
io.grpc.LoadBalancerProvider io.grpc.ManagedChannelProvider io.grpc.NameResolverProvider io.grpc.ServerProvider
cat META-INF/services/*
io.grpc.internal.PickFirstLoadBalancerProvider
io.grpc.util.SecretRoundRobinLoadBalancerProvider$Provider
io.grpc.netty.NettyChannelProvider
io.grpc.netty.UdsNettyChannelProvider
io.grpc.netty.UdsNameResolverProvider
io.grpc.netty.NettyServerProvider
```
**After**
```
ls META-INF/services/
org.sparkproject.connect.grpc.LoadBalancerProvider org.sparkproject.connect.grpc.NameResolverProvider
org.sparkproject.connect.grpc.ManagedChannelProvider org.sparkproject.connect.grpc.ServerProvider
cat META-INF/services/*
org.sparkproject.connect.grpc.internal.PickFirstLoadBalancerProvider
org.sparkproject.connect.grpc.util.SecretRoundRobinLoadBalancerProvider$Provider
org.sparkproject.connect.grpc.protobuf.services.internal.HealthCheckingRoundRobinLoadBalancerProvider
org.sparkproject.connect.grpc.netty.NettyChannelProvider
org.sparkproject.connect.grpc.netty.UdsNettyChannelProvider
org.sparkproject.connect.grpc.netty.UdsNameResolverProvider
org.sparkproject.connect.grpc.internal.DnsNameResolverProvider
org.sparkproject.connect.grpc.netty.NettyServerProvider
```
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092788710
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -105,11 +105,16 @@
<artifactSet>
<includes>
<include>com.google.guava:*</include>
+ <include>io.grpc:*:</include>
Review Comment:
More, I checked the packaging results of the `connect/server` module, It is no longer correct, many packages have not been relocated to `org.sparkproject` package.
I reset the code to https://github.com/apache/spark/pull/38162 and re-checked the packaging results of the `connect/server` module, It's right on that snapshot, all shaded packages are relocated to `org.sparkproject` package.
I'm not sure what changed the packaging results of `server` module, but I think we need to investigate and fix the maven shaded configuration of the `server` modules to ensure that all the shaded packages are relocated to `org.sparkproject` package, I will create a new Jira to tracking this(SPARK-42276).
--
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 #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1407956265
@LuciferYang yes, 3.4 should ship with the JVM client.
--
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] LuciferYang commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1412199826
Could you take a look? @hvanhovell, this one is important
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092643649
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -105,11 +105,16 @@
<artifactSet>
<includes>
<include>com.google.guava:*</include>
+ <include>io.grpc:*:</include>
Review Comment:
@zhenlineo When I run
`mvn clean install -DskipTests -pl connector/connect/client/jvm`
I can see the shaded logs of grpc, and the server side also writes the same
<img width="857" alt="image" src="https://user-images.githubusercontent.com/1475305/215918872-0d5ad3f1-da33-4851-8f74-78e153caacc0.png">
https://github.com/apache/spark/blob/4d37e7816ce83dc5928b0d42a536304cb49eb0a3/connector/connect/server/pom.xml#L244
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092870964
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -105,11 +105,19 @@
<artifactSet>
<includes>
<include>com.google.guava:*</include>
+ <include>io.grpc:*</include>
<include>com.google.protobuf:*</include>
<include>org.apache.spark:spark-connect-common_${scala.binary.version}</include>
</includes>
</artifactSet>
<relocations>
+ <relocation>
Review Comment:
After
```
ls org/sparkproject/connect/client
grpc guava
ls org/sparkproject/connect/client/grpc
Attributes$1.class LoadBalancer$Helper.class
Attributes$Builder.class LoadBalancer$PickResult.class
Attributes$Key.class LoadBalancer$PickSubchannelArgs.class
Attributes.class LoadBalancer$ResolvedAddresses$Builder.class
BinaryLog.class LoadBalancer$ResolvedAddresses.class
BindableService.class LoadBalancer$Subchannel.class
CallCredentials$MetadataApplier.class LoadBalancer$SubchannelPicker.class
CallCredentials$RequestInfo.class LoadBalancer$SubchannelStateListener.class
CallCredentials.class LoadBalancer.class
CallOptions$Key.class LoadBalancerProvider$UnknownConfig.class
....
```
all grpc java class relocation to `org/sparkproject/connect/client/grpc`
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092788710
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -105,11 +105,16 @@
<artifactSet>
<includes>
<include>com.google.guava:*</include>
+ <include>io.grpc:*:</include>
Review Comment:
More, I checked the packaging results of the `connect/server` module, It is no longer correct, many packages have not been relocated to `org.sparkproject` package.
I reset the code to https://github.com/apache/spark/pull/38162 and re-checked the packaging results of the `connect/server` module, It's right on that snapshot, all shaded packages are relocated to `org.sparkproject` package.
I'm not sure what changed the packaging results of `server` module, but I think we need to investigate and fix the maven shaded configuration of the `server` modules to ensure that all the shaded packages are relocated to `org.sparkproject` package, I will create a new Jira to tracking this.
--
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] LuciferYang commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1411506315
> And then the guava is shaded twice
@zhenlineo Do you have time to fix this issue in an independent pr? This seems to not related to the current pr, and this issue is what you found.
--
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 #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1412505539
Merging to master/3.4
--
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] LuciferYang commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1413059936
Thanks @hvanhovell
--
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] LuciferYang commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1409706471
> The reason to use the server shaded jar is because that's the only way to start spark with spark connect.
Why is this the only way? Because the conflict package name of `sql` module and `connect-client-jvm` module?
--
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] LuciferYang commented on a diff in pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #39789:
URL: https://github.com/apache/spark/pull/39789#discussion_r1092832974
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -132,6 +140,10 @@
</includes>
</relocation>
</relocations>
+ <!--SPARK-42228: Add `ServicesResourceTransformer` to relocation class names in META-INF/services for grpc-->
Review Comment:
For fix:
```
org.apache.spark.sql.ClientE2ETestSuite *** ABORTED ***
org.sparkproject.connect.grpc.ManagedChannelProvider$ProviderNotFoundException: No functional channel service provider found. Try adding a dependency on the grpc-okhttp, grpc-netty, or grpc-netty-shaded artifact
at org.sparkproject.connect.grpc.ManagedChannelProvider.provider(ManagedChannelProvider.java:45)
at org.sparkproject.connect.grpc.ManagedChannelBuilder.forAddress(ManagedChannelBuilder.java:39)
at org.apache.spark.sql.connect.client.SparkConnectClient$Builder.build(SparkConnectClient.scala:191)
at org.apache.spark.sql.SparkSession$Builder.<init>(SparkSession.scala:143)
at org.apache.spark.sql.SparkSession$.builder(SparkSession.scala:134)
at org.apache.spark.sql.connect.client.util.RemoteSparkSession.beforeAll(RemoteSparkSession.scala:163)
at org.apache.spark.sql.connect.client.util.RemoteSparkSession.beforeAll$(RemoteSparkSession.scala:160)
at org.apache.spark.sql.ClientE2ETestSuite.beforeAll(ClientE2ETestSuite.scala:22)
at org.scalatest.BeforeAndAfterAll.liftedTree1$1(BeforeAndAfterAll.scala:212)
at org.scalatest.BeforeAndAfterAll.run(BeforeAndAfterAll.scala:210)
...
```
--
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] LuciferYang commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1407953735
If Spark 3.4.0 need release the feature of Spark Connect Java client, I think it is a blocker
--
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] LuciferYang commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1408227661
> @LuciferYang yes, 3.4 should ship with the JVM client.
@hvanhovell Is there a user guide document for the JVM client? I want to know how we want end users to use the JVM client. Thanks ~
--
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] LuciferYang commented on pull request #39789: [SPARK-42228][BUILD][CONNECT] Add shade and relocation rule of grpc to connect-client-jvm module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #39789:
URL: https://github.com/apache/spark/pull/39789#issuecomment-1407611188
also cc @dongjoon-hyun
--
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