[SPARK-44370][CONNECT] Migrate Buf remote generation alpha to remote plugins #41933
Conversation
| @@ -16,18 +16,18 @@ | |||
| # | |||
| version: v1 | |||
| plugins: | |||
| - remote: buf.build/protocolbuffers/plugins/cpp:v3.20.0-1 | |||
| - plugin: buf.build/protocolbuffers/cpp:v21.7 | |||
There was a problem hiding this 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.
There was a problem hiding this 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.
| out: gen/proto/cpp | ||
| - remote: buf.build/protocolbuffers/plugins/csharp:v3.20.0-1 | ||
| - plugin: buf.build/protocolbuffers/csharp:v21.7 |
There was a problem hiding this comment.
also cc @zhengruifeng @panbingkun
|
@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? |
|
Can we try revert the version of buf 1.23.1 and only updating the |
+1, I prefer avoiding such big change just before code freeze |
+1 for me. Let's hold this PR until code freeze. |
|
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. |
|
@Hisoka-X now the buf version is |
Seem like come from this PR protocolbuffers/protobuf@ab4585a, this PR first add |
I think the main reason are updated protobuf version, not related with buf version. After checked, still will make codegen change. |
|
but i think we have pinned the version of protobuf spark/.github/workflows/build_and_test.yml Line 637 in d7bc6f5 |
|
From the test results, it is not a problem with the Buf version. |
If use buf remote plugin, the local pb version will not affected. https://buf.build/docs/generate/usage/#4.-use-remote-plugins |
yeah, this failure actually started before we upgrade buf |
So even using buf 1.20 cannot avoid a lot of code changes, right? @panbingkun |
|
if we can not avoid codegen change, then i am fine to upgrade buf to latest |
Yeah. |
|
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. |
|
cc @grundprinzip would you mind also taking a look? It seems that we need this PR to enable the python codegen |
|
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? |
|
Do we know what the changes in the generated code are? |
|
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. |
It will bring more conflict. And this is a downgrade, it will bring more compatibility issue than v21.7
At now. no
After I check, seem unnecessary. In https://github.com/apache/spark/blob/master/pom.xml#L127 , protobuf version in java already are |
|
Yeah, Java use |
|
I guess it might be related to this: |
|
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? |
|
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 |
|
Merged to master. |
…plugins ### 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. The CI also broken by this reason. ### Why are the changes needed? Migrate Buf remote generation alpha to remote plugins because remote generation alpha features have been sunset. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? exist test Closes apache#41933 from Hisoka-X/SPARK-44370_buf_migrate. Authored-by: Jia Fan <fanjiaeminem@qq.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
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.
The CI also broken by this reason.
Why are the changes needed?
Migrate Buf remote generation alpha to remote plugins because remote generation alpha features have been sunset.
Does this PR introduce any user-facing change?
No
How was this patch tested?
exist test