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

[BEAM-11227] Preparing vendored gRPC upgrade from 1.26 to 1.35 #14028

Closed
wants to merge 36 commits into from

Conversation

suztomo
Copy link
Contributor

@suztomo suztomo commented Feb 20, 2021

This PR prepares the vendored gRPC version upgrade from 1.26 to 1.35

Todo

  • See how this new version breaks compilation. It compiled how about tests?
    • It compiled without changing code manually. Tests have passed except Java PostCommit that has been failing since Feb 27th. (-> sent a mail)
    • With this hack, the pull request checks can run against the vendored gRPC 1.36.0 without publishing it to Maven Central.
  • Have a release manager to publish the vendored gRPC 1.36.0 (http://s.apache.org/beam-release-vendored-artifacts) with votes in dev@beam.apache.
  • Fix anything that fails with the vendored gRPC 1.36.0
  • Remove the directory for the old gRPC 1.26.0
  • Merge this PR.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang SDK Dataflow Flink Samza Spark Twister2
Go Build Status --- Build Status --- Build Status ---
Java Build Status Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status Build Status
Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
--- Build Status ---
XLang Build Status Build Status Build Status --- Build Status ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website Whitespace Typescript
Non-portable Build Status Build Status
Build Status
Build Status
Build Status
Build Status Build Status Build Status Build Status
Portable --- Build Status --- --- --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@codecov
Copy link

codecov bot commented Feb 20, 2021

Codecov Report

Merging #14028 (4f5c13c) into master (08a9d54) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14028      +/-   ##
==========================================
+ Coverage   83.15%   83.16%   +0.01%     
==========================================
  Files         469      938     +469     
  Lines       58972   117948   +58976     
==========================================
+ Hits        49037    98092   +49055     
- Misses       9935    19856    +9921     
Impacted Files Coverage Δ
...m/runners/portability/flink_uber_jar_job_server.py
...uild/srcs/sdks/python/apache_beam/coders/coders.py
...srcs/sdks/python/apache_beam/dataframe/doctests.py
...e_beam/portability/api/beam_runner_api_pb2_urns.py
...srcs/sdks/python/apache_beam/io/gcp/dicomclient.py
...examples/snippets/transforms/aggregation/latest.py
...thon/apache_beam/runners/portability/job_server.py
...ython/apache_beam/io/aws/clients/s3/fake_client.py
...thon/apache_beam/testing/metric_result_matchers.py
...hon/apache_beam/portability/api/schema_pb2_grpc.py
... and 1397 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15ef51f...dc4ac01. Read the comment docs.

@suztomo
Copy link
Contributor Author

suztomo commented Feb 20, 2021

Run Java Precommit

@suztomo
Copy link
Contributor Author

suztomo commented Feb 21, 2021

Run Python_PVR_Flink PreCommit

@kennknowles kennknowles self-requested a review February 22, 2021 23:51
@suztomo
Copy link
Contributor Author

suztomo commented Feb 23, 2021

Discussion in https://issues.apache.org/jira/browse/BEAM-11227 : does this really mark the vendored gRPC artifact out of the CVE?

$ ../../gradlew shadowJar
$ find . -name '*.jar'
./build/libs/beam-vendor-grpc-1_26_0-0.1.jar
$ mvn install:install-file \
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the idea of the command to test the vendored artifact built from the unsubmitted and unpublished code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the purpose of my change. The original command does not seem to install the vendors gRPC artifact locally.

@suztomo
Copy link
Contributor Author

suztomo commented Mar 2, 2021

I'll try to apply the change to use the new vendored gRPC version.


static def guava_version = "30.1-jre"
// protobuf version from https://search.maven.org/artifact/io.grpc/grpc-protobuf/1.36.0/jar
static def protobuf_version = "3.12.0"
Copy link
Contributor Author

@suztomo suztomo Mar 3, 2021

Choose a reason for hiding this comment

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

According to the Linkage Checker (output), this version 3.12.0 suffers from protocolbuffers/protobuf#7827.

java.nio.ByteBuffer's method position(int) is expected to return java.nio.ByteBuffer but instead returns java.nio.Buffer;

class GrpcVendoring_1_36_0 {

static def guava_version = "30.1-jre"
static def protobuf_version = "3.15.3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the latest available version of protobuf-java. We need a version higher than 3.14.0 otherwise it hits Java 8 incompatible problem in protobuf-java 3.13.0. (comment in the ticket)

@suztomo
Copy link
Contributor Author

suztomo commented Mar 3, 2021

GitHub Actions failed:

> Task :sdks:java:core:compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
compiler message file broken: key=compiler.misc.msg.bug arguments=11.0.10, {1}, {2}, {3}, {4}, {5}, {6}, {7}
java.lang.NoSuchMethodError: 'void com.sun.tools.javac.util.Log.error(com.sun.tools.javac.util.JCDiagnostic$DiagnosticPosition, java.lang.String, java.lang.Object[])'
	at com.google.errorprone.ErrorProneError.logFatalError(ErrorProneError.java:55)
	at com.google.errorprone.ErrorProneAnalyzer.finished(ErrorProneAnalyzer.java:155)
	at jdk.compiler/com.sun.tools.javac.api.MultiTaskListener.finished(MultiTaskListener.java:132)
	at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1418)
	at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1365)
	at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:960)
	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.lambda$doCall$0(JavacTaskImpl.java:104)
	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.handleExceptions(JavacTaskImpl.java:147)
	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:100)
	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:94)

This seems the problem in the build (before this PR).

@suztomo
Copy link
Contributor Author

suztomo commented Mar 3, 2021

"Java Tests / Java Unit Tests (macos-latest)" passed.

@suztomo suztomo force-pushed the BEAM-11227_parallel branch from d56edc0 to c1144fc Compare March 3, 2021 21:48
@@ -116,7 +116,7 @@ class PrecommitJobBuilder {
steps {
gradle {
rootBuildScriptDir(commonJobProperties.checkoutDir)
tasks(gradleTask)
tasks(':installVendoredGrpc', gradleTask)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jenkins does not respect this until we tell Jenkins to do so. (That might affect other PRs?)

@@ -40,7 +40,7 @@ PostcommitJobBuilder.postCommitJob('beam_PostCommit_Java', 'Run Java PostCommit'
steps {
gradle {
rootBuildScriptDir(commonJobProperties.checkoutDir)
tasks(':javaPostCommit')
tasks(':installVendoredGrpc', ':javaPostCommit')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jenkins does not respect this until we tell Jenkins to do so. (That might affect other PRs?)

build.gradle.kts Outdated
Comment on lines 339 to 347
// Because :model:job-management:runtimeClasspath requires the vendored gRPC at configuration phase
// (before execution phase), we cannot rely on task dependencies.
if (!project.hasProperty("installVendoredGrpcFlag")) {
project.exec {
commandLine = listOf(
"./gradlew", ":installVendoredGrpc", "-PinstallVendoredGrpcFlag", "--info", "--stacktrace"
)
}
}
Copy link
Contributor Author

@suztomo suztomo Mar 3, 2021

Choose a reason for hiding this comment

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

It worked! (I'll remove this piece of code before merging this to master)

@suztomo
Copy link
Contributor Author

suztomo commented Mar 13, 2021

Run Java Precommit

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run SQL Postcommit

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run Go PreCommit

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run Java_Examples_Dataflow_Java11 PreCommit

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run PythonDocker PreCommit

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run Website_Stage_GCS PreCommit

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run GoPortable PreCommit

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run Python PreCommit

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

"SQL Post Commit Tests" succeeded.

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run Java Postcommit

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run Java HadoopFormatIO Performance Test

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run BigQueryIO Streaming Performance Test Java

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run Dataflow ValidatesRunner

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Run Spark ValidatesRunner

@kennknowles
Copy link
Member

Just to understand more about this, my understanding of the steps would be:

  1. Check in the new vendored gRPC
  2. Release the new vendored gRPC
  3. Check in the migration to the new vendored gRPC
  4. Delete the old vendored gRPC

Does that sound right?

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

@kennknowles Yes, that matches my understanding. Although some tests failed due to timeout, they passed at dd2df58 . I just removed unnecessary build.gradle for testing (The BEAM-11227_parallel fails the GitHub checks because the vendored artifact is not available in Maven Central yet). My branch BEAM-11227_parallel is reach for you to publish the vendored artifact to staging location.

http://s.apache.org/beam-release-vendored-artifacts says:

./gradlew -p vendor/grpc-1_36_0 -PisRelease -PvendoredDependenciesOnly publish

This uploads the artifacts to the staging Apache Nexus repository. Would you perform the staging release?

@kennknowles
Copy link
Member

We should commit the code for the thing before I publish it, though.

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

Sure, let me prepare this PR so that this can be merged to master. (I will need to remove the references to the new vendored classes)

@kennknowles
Copy link
Member

This PR is good for testing but you could just cherrypick the commit with the new vendoring to a separate PR. Up to you!

@suztomo
Copy link
Contributor Author

suztomo commented Mar 15, 2021

@kennknowles I also think another PR is better. I just created #14242

@kennknowles
Copy link
Member

OK we should be able to adjust this to just the changes to move the SDK to the new version.

@suztomo
Copy link
Contributor Author

suztomo commented Mar 22, 2021

@kennknowles I created another PR #14295 (I was reluctant to manage conflicts)

@suztomo
Copy link
Contributor Author

suztomo commented Mar 31, 2021

The other PR 14295 has been merged.

@suztomo suztomo closed this Mar 31, 2021
@suztomo suztomo deleted the BEAM-11227_parallel branch March 31, 2021 16:13
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.

3 participants