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

build(fix): bump-up protobufVersion to 3.21.12 (latest) #760

Merged
merged 3 commits into from Dec 22, 2022

Conversation

laysakura
Copy link
Contributor

Problem

The following command fails in M1 Mac (osx-aarch_64).

./gradlew :rpc:generateProto  # called from `./gradlew test`, for example
> Task :rpc:generateProto FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':rpc:generateProto'.
> Could not resolve all files for configuration ':rpc:protobufToolsLocator_protoc'.
   > Could not find protoc-3.20.3-osx-aarch_64.exe (com.google.protobuf:protoc:3.20.3).
     Searched in the following locations:
         https://repo.maven.apache.org/maven2/com/google/protobuf/protoc/3.20.3/protoc-3.20.3-osx-aarch_64.exe

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/7.0.2/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 646ms
3 actionable tasks: 1 executed, 2 up-to-date

This is because the version 3.20.3 does not include protoc binary for osx-aarch_64 in the maven repository, while 3.21.12 does.

Refs: protocolbuffers/protobuf#8062

What I did

Bump up protobufVersion from 3.20.3 to 3.21.12 (latest).

I'm not sure...

The command:

./gradlew :rpc:generateProto

leads to the following diffs. Currently I don't commit the diffs but I will if necessary.

rpc-generateProto.diff.txt

@brfrn169
Copy link
Collaborator

@laysakura Can you please also commit the diff of the protocol buffers generated classes?

@laysakura
Copy link
Contributor Author

@brfrn169 1d38f11

@brfrn169
Copy link
Collaborator

@laysakura I'm wondering if this change is backward compatible. Did you test it or find any document for it?

@laysakura
Copy link
Contributor Author

@brfrn169 Not yet.

I would like to ask you whether these kind of .java diffs are generated on your environment (in both master branch and this topic branch).

Since:

  • I don't know your team's rule on when to commit proto-generated .java files and
  • I could not generate .java from protobufVersion = 3.20.3,

I'm not sure whether these diffs are usual one or not.

If they usually occur when updating protobufVersion, I will investigate the backward compatibility.

@brfrn169
Copy link
Collaborator

@laysakura These kind of .java diffs usually occur when updating protobufVersion. And we usually commit them when updating protobufVersion.

I could not generate .java from protobufVersion = 3.20.3,

So maybe it's faster to test the compatibility from my side?

@laysakura
Copy link
Contributor Author

@brfrn169 Got it. Thanks.

So maybe it's faster to test the compatibility from my side?

That's true but I would also like to cultivate how to test the proto compatibility in this project.
So how about the following?

  • You kindly test it this time and show me how to do it.
  • I will test the compatibility next time if I have chance.

@brfrn169
Copy link
Collaborator

@laysakura Sure. Let me test it. And I'll show you the result and the procedure. Thanks.

@brfrn169
Copy link
Collaborator

@laysakura Sorry for the late response.

I tested the backward compatibility of this change, and it was successful.

In the test, I started ScalarDB Servers with the change (the build/bumpup-protoc-version2 branch) and ran the integration tests for the servers with clients without the change (the master branch).

The procedure to test the backward compatibility is as follows:

  1. Switch to the target branch (build/bumpup-protoc-version2):
git switch build/bumpup-protoc-version2
  1. Build the ScalarDB Server image:
./gradlew :server:docker
  1. Start the image. We need to start two containers because the integration tests for 2PC transactions need two ScalarDB Servers:
  • Create database1.properties for ScalarDB Server1
scalar.db.contact_points=jdbc:mysql://mysql:3306/
scalar.db.username=root
scalar.db.password=mysql
scalar.db.storage=jdbc

scalar.db.consensus_commit.async_commit.enabled=false

scalar.db.server.port=60051
  • Create database2.properties for ScalarDB Server1
scalar.db.contact_points=jdbc:mysql://mysql:3306/
scalar.db.username=root
scalar.db.password=mysql
scalar.db.storage=jdbc

scalar.db.consensus_commit.async_commit.enabled=false

scalar.db.server.port=60052
  • Create a network
docker network create scalardb-server-network
  • Create a MySQL container in the network
docker run --name mysql --network scalardb-server-network -e MYSQL_ROOT_PASSWORD=mysql -d -p 3306:3306 mysql
  • Start ScalarDB Server1 in the network
docker run --name scalardb-server1 --network scalardb-server-network -v ${PWD}/database1.properties:/scalardb/server/database.properties.tmpl -d -p 60051:60051 ghcr.io/scalar-labs/scalardb-server:4.0.0-SNAPSHOT
  • Start ScalarDB Server2 in the network
docker run --name scalardb-server2 --network scalardb-server-network -v ${PWD}/database2.properties:/scalardb/server/database.properties.tmpl -d -p 60052:60052 ghcr.io/scalar-labs/scalardb-server:4.0.0-SNAPSHOT
  1. Run the integration tests for Server in the master branch
  • Switch to the master branch
git switch master
  • Run the integration tests
./gradlew :server:integrationTestScalarDbServer -Dscalardb.server.external_server_used=true

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@laysakura
Copy link
Contributor Author

@brfrn169 Thank you for your review. I'll merge this PR after the CI pass again.

I also learned how to test the server's backward compatibility.
Is it reasonable to add such integration test as an automated integration test? (then I will do it.)

@brfrn169
Copy link
Collaborator

Is it reasonable to add such integration test as an automated integration test? (then I will do it.)

@laysakura Yes, that's a good idea. You meant to add something a Github actions workflow to test the backward compatibility, right?

@laysakura
Copy link
Contributor Author

@brfrn169 Exactly. I opened an issue: #768

@brfrn169
Copy link
Collaborator

brfrn169 commented Dec 21, 2022

@brfrn169 Exactly. I opened an issue: #768

@laysakura Thank you!

Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@feeblefakie feeblefakie merged commit 41eb19c into master Dec 22, 2022
@feeblefakie feeblefakie deleted the build/bumpup-protoc-version2 branch December 22, 2022 02:26
@brfrn169 brfrn169 added this to Done in 3.8.0 Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
3.8.0
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants