core, inprocess, util: move inprocess and util code into their own new artifacts grpc-inprocess and grpc-util #10362

sanjaypujare · 2023-07-09T19:00:13Z

CC @ejona86

…w artifacts grpc-inprocess and grpc-util

sanjaypujare · 2023-07-10T17:54:53Z

Thanks @larry-safran . Now waiting for @ejona86 's LGTM....

sanjaypujare · 2023-07-12T00:00:35Z

Gentle ping @ejona86 - waiting for your LGTM

ejona86 · 2023-07-13T21:10:32Z

core/BUILD.bazel

 java_library(
    name = "internal",
+    visibility = ["//visibility:public"],


internal should not be public. Why did you need this? It was already allowing subpackages.

ejona86 · 2023-07-13T21:32:30Z

core/src/main/resources/META-INF/services/io.grpc.LoadBalancerProvider

@@ -1,3 +1 @@
 io.grpc.internal.PickFirstLoadBalancerProvider
-io.grpc.util.SecretRoundRobinLoadBalancerProvider$Provider


RR moving to grpc-util could definitely break users in a hard-to-diagnose way. I'm not sure how we want to handle this. Short-term we could have a runtimeDependency in grpc-core onto grpc-util.

That would mean we'd also want //inprocess in core_maven

... Short-term we could have a runtimeDependency in grpc-core onto grpc-util.

Interesting. grpc-util has an api dependency on grpc-core so doing what you suggest makes the dependency circular (even with runtimeOnly dependency). Any ideas?

Looks like this is a blocker and needs to be resolved. So I'll wait for that before I address the remaining comments.

I was aware that would be the case. But it should work. Are you saying it doesn't work?

Yes, I had tried it before I added the comment:

* What went wrong: Circular dependency between the following tasks: :grpc-core:checkUpperBoundDeps +--- :grpc-core:jar | +--- :grpc-core:classes | | \--- :grpc-core:compileJava | | \--- :grpc-core:checkUpperBoundDeps (*) | \--- :grpc-core:compileJava (*) \--- :grpc-util:jar +--- :grpc-util:classes | \--- :grpc-util:compileJava | +--- :grpc-core:compileJava (*) | \--- :grpc-util:checkUpperBoundDeps | +--- :grpc-core:jar (*) | \--- :grpc-util:jar (*) \--- :grpc-util:compileJava (*)

This has been resolved

hi i just wanted to let you know that this did indeed break for us in a hard to diagnose way. Is there a correct way to upgrade this library so that it does not break? in general we use something like

ManagedChannelBuilder .forTarget(URL) .defaultLoadBalancingPolicy("round_robin") .usePlaintext() .build()

@mdering "break" is not a helpful description. Please open a new issue and describe the error/problem you see when upgrading. You can link to this PR. For this specific change, it's also important whether you are shading/shadowing/making a fat jar.

ejona86 · 2023-07-13T21:41:57Z

core/BUILD.bazel

@@ -4,33 +4,16 @@ java_library(
    exports = ["//api"],
 )

-java_library(
-    name = "inprocess",


Leave a forwarding library in place. Keep name and visibility and have it export //inprocess. Ditto for util.

ejona86 · 2023-07-13T21:42:30Z

inprocess/build.gradle

+components.java.withVariantsFromConfiguration(configurations.testFixturesRuntimeElements) { skip() }
+
+
+tasks.register("versionFile") {


Keep this in core. It is not needed in inprocess and util.

ejona86 · 2023-07-13T21:43:35Z

inprocess/build.gradle

+    exclude 'io/grpc/inprocess/Internal*'
+}
+
+animalsniffer {


Delete. There's no jmh.

ejona86 · 2023-07-13T21:50:14Z

inprocess/build.gradle

+            testFixtures(project(':grpc-core'))
+    testImplementation libraries.guava.testlib
+
+    testRuntimeOnly project(':grpc-census')


ejona86 · 2023-07-13T21:52:24Z

repositories.bzl

@@ -79,6 +79,8 @@ IO_GRPC_GRPC_JAVA_OVERRIDE_TARGETS = {
    "io.grpc:grpc-stub": "@io_grpc_grpc_java//stub",
    "io.grpc:grpc-testing": "@io_grpc_grpc_java//testing",
    "io.grpc:grpc-xds": "@io_grpc_grpc_java//xds:xds_maven",
+    "io.grpc:grpc-inprocess": "@io_grpc_grpc_java//inprocess",


Sort these.

ejona86 · 2023-07-13T21:52:52Z

rls/build.gradle

@@ -9,8 +9,8 @@ plugins {
 description = "gRPC: RouteLookupService Loadbalancing plugin"

 dependencies {
-    implementation project(':grpc-core'),
-            project(':grpc-protobuf'),
+    api project(':grpc-util')


This should be an implementation dependency.

ejona86 · 2023-07-13T21:55:15Z

util/BUILD.bazel

+# with maven_install's override_targets. Should only be used as a dep for
+# pre-compiled binaries on Maven Central.
+java_library(
+    name = "util_maven",


There's no need to export //api from here, so delete the entire target. If it were needed, it should have been used from repositories.bzl

ejona86 · 2023-07-13T21:57:47Z

util/src/bazel-util/resources/META-INF/services/io.grpc.LoadBalancerProvider

Delete this file and use src/main/resources instead.

ejona86

Most of my earlier comments apply to util/build.gradle as well.

ejona86 · 2023-07-14T23:01:13Z

grpclb/build.gradle

@@ -10,7 +10,7 @@ plugins {
 description = "gRPC: GRPCLB LoadBalancer plugin"

 dependencies {
-    implementation project(':grpc-core'),
+    implementation project(':grpc-inprocess'),


Why was it changed to depend on inprocess?

ejona86 · 2023-07-14T23:03:50Z

util/build.gradle

+dependencies {
+    api project(':grpc-core')
+
+    implementation libraries.gson,


Why depend on gson?

Without it the build of grpc-services fails:

* What went wrong: Execution failed for task ':grpc-services:checkUpperBoundDeps'. > Maven version skew: com.google.code.gson:gson (2.8.9 != 2.10.1) Bad version dependency path: [project ':grpc-services', com.google.protobuf:protobuf-java-util:3.22.3] Run './gradlew :grpc-services:dependencies --configuration runtimeClasspath' to diagnose

And then running ./gradlew :grpc-services:dependencies --configuration runtimeClasspath (as suggested) gives me:

runtimeClasspath - Runtime classpath of source set 'main'. ... +--- project :grpc-util | +--- project :grpc-core | | +--- project :grpc-api (*) | | +--- com.google.code.gson:gson:2.10.1 ===> v2.10.1 referenced | | +--- com.google.android:annotations:4.1.1.4 | | +--- org.codehaus.mojo:animal-sniffer-annotations:1.23 | | +--- com.google.errorprone:error_prone_annotations:2.18.0 | | +--- com.google.guava:guava:32.0.1-android -> 32.0.1-jre (*) | | +--- io.perfmark:perfmark-api:0.26.0 | | +--- project :grpc-context | | | \--- project :grpc-api (*) | | \--- project :grpc-util (*) | +--- org.codehaus.mojo:animal-sniffer-annotations:1.23 | \--- com.google.guava:guava:32.0.1-android -> 32.0.1-jre (*) +--- com.google.protobuf:protobuf-java-util:3.22.3 | +--- com.google.protobuf:protobuf-java:3.22.3 | +--- com.google.code.findbugs:jsr305:3.0.2 | +--- com.google.code.gson:gson:2.8.9 -> 2.10.1 ===> v2.8.9 referenced | +--- com.google.errorprone:error_prone_annotations:2.11.0 -> 2.18.0 | +--- com.google.guava:guava:31.1-jre -> 32.0.1-jre (*) | \--- com.google.j2objc:j2objc-annotations:1.3 -> 2.8 ...

I can also fix it by making libraries.gson a runtimeOnly dependency of grpc-util and then add a comment describing why it is required?

I can also fix it by making libraries.gson a runtimeOnly dependency of grpc-util and then add a comment describing why it is required?

Yes. Do that for forced upgrading to fix version skew.

Already done with a comment

util/build.gradle

ejona86 · 2023-07-14T23:05:30Z

core/build.gradle

@@ -23,7 +23,8 @@ description = 'gRPC: Core'
 dependencies {
    api project(':grpc-api')
    // force dependent jars to depend on latest grpc-context
-    runtimeOnly project(":grpc-context")
+    runtimeOnly project(":grpc-context"),
+            project(":grpc-util")


Include a comment that we depend on grpc-util to pull in RR

core/build.gradle

…n grpc-services

ejona86

LGTM

ejona86 · 2023-07-18T15:42:53Z

core/BUILD.bazel

        "//api",
+        "//inprocess",


This should have exported //util, not //inprocess (to mirror core/build.gradle)

ejona86 · 2023-07-18T15:47:02Z

util/build.gradle

+
+    implementation libraries.animalsniffer.annotations,
+            libraries.guava
+    runtimeOnly libraries.gson  // to fix checkUpperBoundDeps error in services


No, this should go in services.

grpc-core does not contain the inprocess classes anymore, they were moved out to io.grpc:grpc-inprocess so we need to add it as a dependency. See https://github.com/grpc/grpc-java/releases/tag/v1.58.0 See grpc/grpc-java#10362 Closes gh-4063

core, inprocess, util: move inprocess and util code into their own ne…

743edeb

…w artifacts grpc-inprocess and grpc-util

sanjaypujare requested a review from larry-safran July 9, 2023 19:00

multiple: fix bazel build

0abd922

larry-safran approved these changes Jul 10, 2023

View reviewed changes

ejona86 self-requested a review July 13, 2023 20:30

ejona86 reviewed Jul 13, 2023

View reviewed changes

address review comments

ccd6a4b

ejona86 reviewed Jul 14, 2023

View reviewed changes

clean up grpc-util along the same lines as grpc-inprocess

0b30c31

ejona86 reviewed Jul 14, 2023

View reviewed changes

sanjaypujare added 3 commits July 14, 2023 16:44

remove unnecessary deps in grpclb and util

50f9552

util: need explicit dep on gson otherwise checkUpperBoundDeps fails i…

ab7a652

…n grpc-services

make gson a runtimeOnly dependency

662884b

sanjaypujare merged commit 0f5f07f into grpc:master Jul 17, 2023

sanjaypujare deleted the java9-core-split branch July 17, 2023 18:45

ejona86 reviewed Jul 18, 2023

View reviewed changes

sanjaypujare mentioned this pull request Jul 18, 2023

core, services, util: export util from core and move gson dep to services for the checkUpperBoundDeps error #10390

Merged

jonatan-ivanov mentioned this pull request Sep 7, 2023

Migrate to grpc-java 1.58.0 micrometer-metrics/micrometer#4063

Closed

yannickepstein mentioned this pull request Dec 15, 2023

Add grpc-util as compile dependency to grpc-xds #10755

Closed

github-actions bot locked as resolved and limited conversation to collaborators Dec 28, 2023

		@@ -1,3 +1 @@
		io.grpc.internal.PickFirstLoadBalancerProvider
		io.grpc.util.SecretRoundRobinLoadBalancerProvider$Provider

		components.java.withVariantsFromConfiguration(configurations.testFixturesRuntimeElements) { skip() }


		tasks.register("versionFile") {

core, inprocess, util: move inprocess and util code into their own new artifacts grpc-inprocess and grpc-util #10362

core, inprocess, util: move inprocess and util code into their own new artifacts grpc-inprocess and grpc-util #10362

Uh oh!

Conversation

sanjaypujare commented Jul 9, 2023

Uh oh!

sanjaypujare commented Jul 10, 2023

Uh oh!

sanjaypujare commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sanjaypujare commented Jul 12, 2023 •

edited

Loading