Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use older protobuf/grpc codegen to support older runtimes #4000

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Jun 22, 2023

Motivation

It seems that protobuf and grpc codegen should be done using a codegen version of the oldest version
of protobuf and grpc-java to support at runtime. Generated code is designed to be compatible with newer runtimes,
but not necessarily compatible with older runtimes. This is explained in comment grpc/grpc-java#10202 (comment) .

Changes

Set protoc3.version to 3.19.6.
Set protoc-gen-grpc-java.version to 1.45.1.

@lhotari
Copy link
Member Author

lhotari commented Jun 22, 2023

@zymap Please review this.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@eolivelli
Copy link
Contributor

The build fails

[INFO] ------------------------------------------------------------------------
Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.10.1:compile (default-compile) on project stream-storage-tests-common: Compilation failure: Compilation failure: 
Error:  /home/runner/work/bookkeeper/bookkeeper/stream/tests-common/target/generated-sources/protobuf/java/org/apache/bookkeeper/stream/coder/protobuf/test/Proto2CoderTestMessages.java:[1803,29] name clash: clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<org.apache.bookkeeper.stream.coder.protobuf.test.Proto2CoderTestMessages.MessageC,?>) in org.apache.bookkeeper.stream.coder.protobuf.test.Proto2CoderTestMessages.MessageC.Builder and clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<org.apache.bookkeeper.stream.coder.protobuf.test.Proto2CoderTestMessages.MessageC,T>) in com.google.protobuf.GeneratedMessageV3.ExtendableBuilder have the same erasure, yet neither overrides the other
Error:  /home/runner/work/bookkeeper/bookkeeper/stream/tests-common/target/generated-sources/protobuf/java/org/apache/bookkeeper/stream/coder/protobuf/test/Proto2CoderTestMessages.java:[1802,7] method does not override or implement a method from a supertype
Error:  -> [Help 1]
Error:  

@eolivelli eolivelli self-requested a review June 22, 2023 12:40
@lhotari
Copy link
Member Author

lhotari commented Jun 22, 2023

I now found protobuf cross-version runtime guarantees:
https://protobuf.dev/support/cross-version-runtime-guarantee/

Protobuf will not support mixing generated code and runtimes across major version boundaries. We will add “poison pills” where possible to detect these mismatches.

Therefore this PR isn't currently valid. There will have to be an additional requirement for bookkeeper clients that they use a grpc & protobuf version at runtime which is compatible with the one used in bookkeeper client. There doesn't seem to be workarounds.

@lhotari
Copy link
Member Author

lhotari commented Jun 22, 2023

Closing this PR since the solution doesn't work.

@lhotari lhotari closed this Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants