You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "Hisoka-X (via GitHub)" <gi...@apache.org> on 2023/07/11 06:20:44 UTC
[GitHub] [spark] Hisoka-X opened a new pull request, #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Hisoka-X opened a new pull request, #41933:
URL: https://github.com/apache/spark/pull/41933
<!--
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?
Buf unsupported remote generation alpha at now. Please refer https://buf.build/docs/migration-guides/migrate-remote-generation-alpha/ . We should migrate Buf remote generation alpha to remote plugins by follow the guide.
<!--
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?
Migrate Buf remote generation alpha to remote plugins because remote generation alpha features have been sunset.
<!--
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?
No
<!--
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?
exist test
<!--
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 pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630397278
> From the test results, it is not a problem with the Buf version. <img alt="image" width="906" src="https://user-images.githubusercontent.com/15246973/252596321-cbb55024-8cb1-4865-9009-c40846163167.png">
So even using buf 1.20 cannot avoid a lot of code changes, right? @panbingkun
--
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] grundprinzip commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630653264
I think using the 21.7 version might not be very risky, but we should make sure that we stay consistent in our protobuf usage and we need to check if this should apply as well to upgrading the java version.
--
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] Hisoka-X commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630214344
cc @HyukjinKwon @grundprinzip
--
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] zhengruifeng commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630579815
cc @grundprinzip would you mind also taking a look? It seems that we need this PR to enable the python codegen
--
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] zhengruifeng commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630402998
if we can not avoid codegen change, then i am fine to upgrade buf to latest
--
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 #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630296883
Can we try revert the version of buf 1.23.1 and only updating the `remote-plugins`? If don't need to change so much code, I think we can revert buf version before code freeze
--
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] panbingkun commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630410471
> > From the test results, it is not a problem with the Buf version. <img alt="image" width="906" src="https://user-images.githubusercontent.com/15246973/252596321-cbb55024-8cb1-4865-9009-c40846163167.png">
>
> So even using buf 1.20 cannot avoid a lot of code changes, right? @panbingkun
Yeah.
<img width="576" alt="image" src="https://github.com/apache/spark/assets/15246973/1dcf5dd0-bae5-4482-8edd-a037ef350710">
--
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 closed pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
URL: https://github.com/apache/spark/pull/41933
--
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] zhengruifeng commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630326822
@Hisoka-X now the buf version is `v1.20.0`, would you mind help checking whether this migration still cause codegen changes?
--
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] Hisoka-X commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630359064
> but i think we have pinned the version of protobuf
>
> https://github.com/apache/spark/blob/d7bc6f5c7efd9dcb1e460657447d1a9ea8f03f62/.github/workflows/build_and_test.yml#L637
If use buf remote plugin, the local pb version will not affected. https://buf.build/docs/generate/usage/#4.-use-remote-plugins
--
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] grundprinzip commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1631004094
TBH I don't know how exactly to fix this problem. The buf remote plugins don't offer the protobuf version we need, so maybe the fix is to switch from remote to local plugins.
--
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] grundprinzip commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630651148
Do we know what the changes in the generated code are?
--
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 #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #41933:
URL: https://github.com/apache/spark/pull/41933#discussion_r1259300182
##########
connector/connect/common/src/main/buf.gen.yaml:
##########
@@ -16,18 +16,18 @@
#
version: v1
plugins:
- - remote: buf.build/protocolbuffers/plugins/cpp:v3.20.0-1
+ - plugin: buf.build/protocolbuffers/cpp:v21.7
out: gen/proto/cpp
- - remote: buf.build/protocolbuffers/plugins/csharp:v3.20.0-1
+ - plugin: buf.build/protocolbuffers/csharp:v21.7
Review Comment:
also cc @zhengruifeng @panbingkun
--
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] zhengruifeng commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630290786
@LuciferYang @panbingkun @Hisoka-X @HyukjinKwon
do you know what cause this large changes in generated codes? the upgrade to v1.23.1? or this migration?
--
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] panbingkun commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630352345
From the test results, it is not a problem with the Buf version.
<img width="906" alt="image" src="https://github.com/apache/spark/assets/15246973/cbb55024-8cb1-4865-9009-c40846163167">
--
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] zhengruifeng commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630344412
but i think we have pinned the version of protobuf https://github.com/apache/spark/blob/d7bc6f5c7efd9dcb1e460657447d1a9ea8f03f62/.github/workflows/build_and_test.yml#L637
--
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] Hisoka-X commented on a diff in pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on code in PR #41933:
URL: https://github.com/apache/spark/pull/41933#discussion_r1259244060
##########
connector/connect/common/src/main/buf.gen.yaml:
##########
@@ -16,18 +16,18 @@
#
version: v1
plugins:
- - remote: buf.build/protocolbuffers/plugins/cpp:v3.20.0-1
+ - plugin: buf.build/protocolbuffers/cpp:v21.7
Review Comment:
I'm not sure the version should be updated to this, but this (and v3.14.0) is the most older version maintained by the Buf team.
--
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] zhengruifeng commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630365221
>
yeah, this failure actually started before we upgrade buf
--
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] Hisoka-X commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1631016514
> TBH I don't know how exactly to fix this problem. The buf remote plugins don't offer the protobuf version we need, so maybe the fix is to switch from remote to local plugins.
I'm not sure what the downsides are, but it certainly makes packaging and testing more cumbersome for developers. But it is indeed a feasible solution to the current problem. @HyukjinKwon @zhengruifeng @LuciferYang @panbingkun 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] Hisoka-X commented on a diff in pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on code in PR #41933:
URL: https://github.com/apache/spark/pull/41933#discussion_r1259320039
##########
connector/connect/common/src/main/buf.gen.yaml:
##########
@@ -16,18 +16,18 @@
#
version: v1
plugins:
- - remote: buf.build/protocolbuffers/plugins/cpp:v3.20.0-1
+ - plugin: buf.build/protocolbuffers/cpp:v21.7
out: gen/proto/cpp
- - remote: buf.build/protocolbuffers/plugins/csharp:v3.20.0-1
+ - plugin: buf.build/protocolbuffers/csharp:v21.7
out: gen/proto/csharp
- - remote: buf.build/protocolbuffers/plugins/java:v3.20.0-1
+ - plugin: buf.build/protocolbuffers/java:v21.7
out: gen/proto/java
- plugin: buf.build/grpc/ruby:v1.56.0
out: gen/proto/ruby
- - remote: buf.build/protocolbuffers/plugins/ruby:v21.2.0-1
+ - plugin: buf.build/protocolbuffers/ruby:v21.7
out: gen/proto/ruby
# Building the Python build and building the mypy interfaces.
- - remote: buf.build/protocolbuffers/plugins/python:v3.19.3-1
+ - plugin: buf.build/protocolbuffers/python:v21.7
Review Comment:
I think choose minor update is better every time, not bring too big version update. Because this PR main purpose is migrate remote plugin.
--
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] Hisoka-X commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630328500
> @LuciferYang @panbingkun @Hisoka-X @HyukjinKwon
>
> do you know what cause this large changes in generated codes? the upgrade to v1.23.1? or this migration?
Seem like come from this PR https://github.com/protocolbuffers/protobuf/commit/ab4585a6956675ce14a1cba5d321fde980bbf12b, this PR first add `BuildMessageAndEnumDescriptors` method. But don't have enough information. cc @zhengruifeng
--
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] panbingkun commented on a diff in pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41933:
URL: https://github.com/apache/spark/pull/41933#discussion_r1259319890
##########
connector/connect/common/src/main/buf.gen.yaml:
##########
@@ -16,18 +16,18 @@
#
version: v1
plugins:
- - remote: buf.build/protocolbuffers/plugins/cpp:v3.20.0-1
+ - plugin: buf.build/protocolbuffers/cpp:v21.7
Review Comment:
Maybe as follow:
- plugin: buf.build/grpc/cpp:v1.56.0
out: gen/proto/cpp
- plugin: buf.build/protoco
out: gen/proto/cpp
Its writing is consistent with the original plugin's Ruby.
--
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] panbingkun commented on a diff in pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41933:
URL: https://github.com/apache/spark/pull/41933#discussion_r1259307304
##########
connector/connect/common/src/main/buf.gen.yaml:
##########
@@ -16,18 +16,18 @@
#
version: v1
plugins:
- - remote: buf.build/protocolbuffers/plugins/cpp:v3.20.0-1
+ - plugin: buf.build/protocolbuffers/cpp:v21.7
out: gen/proto/cpp
- - remote: buf.build/protocolbuffers/plugins/csharp:v3.20.0-1
+ - plugin: buf.build/protocolbuffers/csharp:v21.7
out: gen/proto/csharp
- - remote: buf.build/protocolbuffers/plugins/java:v3.20.0-1
+ - plugin: buf.build/protocolbuffers/java:v21.7
out: gen/proto/java
- plugin: buf.build/grpc/ruby:v1.56.0
out: gen/proto/ruby
- - remote: buf.build/protocolbuffers/plugins/ruby:v21.2.0-1
+ - plugin: buf.build/protocolbuffers/ruby:v21.7
out: gen/proto/ruby
# Building the Python build and building the mypy interfaces.
- - remote: buf.build/protocolbuffers/plugins/python:v3.19.3-1
+ - plugin: buf.build/protocolbuffers/python:v21.7
Review Comment:
https://buf.build/protocolbuffers/python
plugin: buf.build/protocolbuffers/python:v23.4
It seems that this version is the latest.
--
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] grundprinzip commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1631139062
My suggestion would be to merge the proposed version with v21.7 and assuming that the integration test pass should be still compatible.
This should unblock any new proto changes.
--
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] Hisoka-X commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630707847
> What is the diff if we use the 3.14 version of protobuf?
It will bring more conflict. And this is a downgrade, it will bring more compatibility issue than v21.7
> Do we know what the changes in the generated code are?
At now. no
> we need to check if this should apply as well to upgrading the java version.
I will do it. Thanks for remind.
--
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] panbingkun commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1631671090
@Hisoka-X Local plugins:
https://buf.build/docs/generate/usage/#3.-use-local-plugins
--
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] Hisoka-X commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630331397
> @Hisoka-X now the buf version is `v1.20.0`, would you mind help checking whether this migration still cause codegen changes?
I think the main reason are updated protobuf version, not related with buf version. But let me confirm it.
--
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] zhengruifeng commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630299746
> Can we try revert the version of buf 1.23.1 and only updating the `remote-plugins`? If don't need to change so much code, I think we can revert buf version before code freeze
+1, I prefer avoiding such big change just before code freeze
--
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] Hisoka-X commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630301287
> Can we try revert the version of buf 1.23.1 and only updating the `remote-plugins`? If don't need to change so much code, I think we can revert buf version before code freeze
+1 for me. Let's hold this PR until code freeze.
--
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] Hisoka-X commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630318506
> Let's try revert buf.
I'm not sure it will work or not, because buf version is 1.17.0 in my local. Also will return error.
--
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 #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630718040
Yeah, Java use `sbt-protoc` or `protobuf-maven-plugin`, not `buf`
--
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] panbingkun commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630951628
I guess it might be related to this:
https://buf.build/blog/breaking-change-governance/
<img width="867" alt="image" src="https://github.com/apache/spark/assets/15246973/37a04230-22a8-4c33-a577-d0c1ca7395e5">
--
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] grundprinzip commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630637888
We have to be careful that the remotely generated protobuf code matches our protobuf version locally because otherwise we will run into compatibility issues. Protobuf had some changes which might cause the large diff in this PR.
What is the diff if we use the 3.14 version of protobuf?
--
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] Hisoka-X commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1631700584
> My suggestion would be to merge the proposed version with v21.7 and assuming that the integration test pass should be still compatible.
>
> This should unblock any new proto changes.
As you said. The CI passed, I think we can use remote plugin for v21.7 now. cc @panbingkun
--
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] Hisoka-X commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1631846601
Thanks @HyukjinKwon @grundprinzip @zhengruifeng @LuciferYang @panbingkun
--
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 #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1631839180
Merged to master.
--
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] panbingkun commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630313680
Let's try revert buf.
--
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] Hisoka-X commented on pull request #41933: [SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41933:
URL: https://github.com/apache/spark/pull/41933#issuecomment-1630505708
Hi @zhengruifeng @HyukjinKwon @LuciferYang , the CI passed. I think if we don't have another way to fix CI, maybe we can merge this PR, by the way the change only will affect 3.5.0. If don't want too many codegen changed before code freeze, disable this CI check temporary also will be an option.
--
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