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
Upgrade gRPC to support Apple Silicon #13
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,11 +40,12 @@ | |
<commons-lang3.version>3.10</commons-lang3.version> | ||
<commons-codec.version>1.9</commons-codec.version> | ||
<codehaus.jackson.version>1.9.13</codehaus.jackson.version> | ||
<error_prone_annotations.version>2.3.4</error_prone_annotations.version> | ||
<error_prone_annotations.version>2.10.0</error_prone_annotations.version> | ||
<execution.root>${user.dir}</execution.root> | ||
<fasterxml.jackson.version>2.10.0</fasterxml.jackson.version> | ||
<grpc.version>1.33.0</grpc.version> | ||
<guava.version>30.0-jre</guava.version> | ||
<grpc.version>1.47.0</grpc.version> | ||
<gson.version>2.9.0</gson.version> | ||
<guava.version>31.0.1-jre</guava.version> | ||
<hadoop.scope>provided</hadoop.scope> | ||
<hadoop.version>2.8.5</hadoop.version> | ||
<httpclient.version>4.5.3</httpclient.version> | ||
|
@@ -64,8 +65,7 @@ | |
<picocli.version>4.5.2</picocli.version> | ||
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
<prometheus.simpleclient.version>0.9.0</prometheus.simpleclient.version> | ||
<protobuf.version>3.12.0</protobuf.version> | ||
<protoc.version>3.12.0</protoc.version> | ||
<protobuf.version>3.19.2</protobuf.version> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems native M1 support was introduced in v3.20.1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not exactly, it is from v3.17.3 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3.17.3 is still Rosetta, there is only protoc-3.17.3-osx-x86_64.zip. In 3.20.1, there is protoc-3.20.1-osx-aarch_64.zip. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should check the maven repo instead of GitHub release There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the link, seems they both requires Rosetta 2. $ file protoc-3.17.3-osx-aarch_64.exe protoc-3.20.1-osx-aarch_64.exe
protoc-3.17.3-osx-aarch_64.exe: Mach-O 64-bit executable x86_64
protoc-3.20.1-osx-aarch_64.exe: Mach-O 64-bit executable x86_64 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's a funny thing, they just copy and rename the file... both file has the same md5 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The latest version is right
|
||
<roaring.bitmap.version>0.9.15</roaring.bitmap.version> | ||
<rss.shade.packageName>org.apache.uniffle</rss.shade.packageName> | ||
<skipDeploy>false</skipDeploy> | ||
|
@@ -184,6 +184,13 @@ | |
</exclusion> | ||
</exclusions> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>com.google.code.gson</groupId> | ||
<artifactId>gson</artifactId> | ||
<version>${gson.version}</version> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>io.netty</groupId> | ||
<artifactId>netty-all</artifactId> | ||
|
@@ -588,7 +595,7 @@ | |
<version>0.6.1</version> | ||
<configuration> | ||
<protocArtifact> | ||
com.google.protobuf:protoc:${protoc.version}:exe:${os.detected.classifier} | ||
com.google.protobuf:protoc:${protobuf.version}:exe:${os.detected.classifier} | ||
</protocArtifact> | ||
<pluginId>grpc-java</pluginId> | ||
<pluginArtifact> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we also bump guava and gson version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
w/o upgrading gson and guava, it will violate rules defined by
maven-enforcer-plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grpc and protobuf should work well w/ the previous version, the dependency upgrading is trivial
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to make the changes minimal to avoid unexpected issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should remove
requireUpperBoundDeps
rule ofmaven-enforcer-plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minimal change was tried in Tencent/Firestorm#185, but there were some issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found and fixed some issues in Spark3 IT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's actually the minimal change to meet
requireUpperBoundDeps
requirements.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for digging out this issue, I remember the same issue was encountered during previous attempts.