feat: migrate grpc-gcp to maven#237
Conversation
rahul2393
force-pushed
the
migrate-grpc-gcp-to-maven
branch
from
8658f91 to
447e1f6
Compare
|
|
||
| public class GrpcGcpUtil { | ||
| public static final String IMPLEMENTATION_VERSION = "1.10.0"; | ||
| public static final String IMPLEMENTATION_VERSION = "1.11.0-SNAPSHOT"; |
There was a problem hiding this comment.
How are we going to keep this in sync with the pom.xml? And do we really need it? Is it used in actual production code, or only in tests? In the latter case, there are probably better solutions than to put it in code. In PGAdapter we have something similar here: https://github.com/GoogleCloudPlatform/pgadapter/blob/d476689dc22670ee227fe65714d1fa849d761ab7/src/main/java/com/google/cloud/spanner/pgadapter/Server.java#L371
There was a problem hiding this comment.
This is used for the OpenTelemetry instrumentation scope version in production, updating this to what we do in java-spanner where we use GaxProperties.getLibraryVersion(getClass()), which reads Package#getImplementationVersion() from the JAR manifest and falls back to "".
| public Object parse(InputStream stream) { | ||
| try { | ||
| return stream.readAllBytes().toString(); | ||
| ByteArrayOutputStream buffer = new ByteArrayOutputStream(); |
There was a problem hiding this comment.
Why do we need to change this for a migration from Gradle to Maven?
There was a problem hiding this comment.
This and other changes are here because, google-cloud-java / sdk-platform-java-config enforces Java 8 compatibility, Maven parent compiles with Java 8 release target (--release 8).
InputStream.readAllBytes() is Java 9+.
So code must avoid it before the module can fit google-cloud-java build.
| .dropWhile(chRef -> prevChannels.contains(chRef)) | ||
| .findFirst() | ||
| .get(); | ||
| pool.channelRefs.stream().filter(chRef -> !prevChannels.contains(chRef)).findFirst().get(); |
There was a problem hiding this comment.
I think this does the same as the previous code (?) But could we maybe keep this PR to only the Gradle-to-Maven migration? Same also applies to the other code changes here that are not related to the actual migration. (Meaning: I'm not looking into any of the other code changes in this PR, except where I think it might actually be related to the migration)
There was a problem hiding this comment.
Stream#dropWhile is Java 9+, while sdk-platform-java-config enforces Java 8 APIs with --release 8.
| <protobuf.version>4.33.2</protobuf.version> | ||
| <grpc.version>1.81.0</grpc.version> | ||
| <opencensus.version>0.31.1</opencensus.version> | ||
| <bigtable.version>2.77.1</bigtable.version> | ||
| <spanner.version>6.117.0</spanner.version> | ||
| <truth.version>1.4.5</truth.version> | ||
| <mockito.version>5.23.0</mockito.version> |
There was a problem hiding this comment.
I assume that these versions are the same as what they were in the Gradle build file?
There was a problem hiding this comment.
No, we are intentionally updating these to the versions used by the current Cloud Java build (sdk-platform-java-config / google-cloud-shared-dependencies) rather than preserving the old Gradle versions, because the next step is moving this module into google-cloud-java. The other version changes are intentional alignment with the target repo.
| <parent> | ||
| <groupId>com.google.cloud</groupId> | ||
| <artifactId>sdk-platform-java-config</artifactId> | ||
| <version>3.62.0</version><!-- {x-version-update:google-cloud-shared-dependencies:current} --> |
There was a problem hiding this comment.
Is the {x-version-update:google-cloud-shared-dependencies:current} correct here? This is not the reference to the shared-dependencies pom. It is a reference to sdk-platform-java.
There was a problem hiding this comment.
This is intentional
Reference: https://github.com/googleapis/google-cloud-java/blob/main/sdk-platform-java/sdk-platform-java-config/pom.xml#L21
There was a problem hiding this comment.
we will update it google-cloud-jar-parent when we move to monorepo right?
| <dependency> | ||
| <groupId>com.google.cloud</groupId> | ||
| <artifactId>google-cloud-shared-dependencies</artifactId> | ||
| <version>${google-cloud-shared-dependencies.version}</version> |
There was a problem hiding this comment.
Is this property defined in the parent?
There was a problem hiding this comment.
Yes, defined by parent sdk-platform-java-config.
| <type>pom</type> | ||
| <scope>import</scope> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Shouldn't this be the spanner-bom, and type=pom, scope=import?
There was a problem hiding this comment.
And why do we need to import it here anyways? Same also for the bigtable-bom, why is it needed in grpc-gcp?
There was a problem hiding this comment.
Those are here only for test dependencies, Integration tests directly use Spanner + Bigtable client/proto/grpc artifacts. BOMs keep these related test artifacts on one version.
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.google.cloud</groupId> |
There was a problem hiding this comment.
Ah, OK. I assume that this is the reason that we import the pom in the dependency management section.
| public Object parse(InputStream stream) { | ||
| try { | ||
| return stream.readAllBytes().toString(); | ||
| ByteArrayOutputStream buffer = new ByteArrayOutputStream(); |
4 participants
Migrate
grpc-gcpbuild from Gradle to MavenReplace the Gradle build with a Maven
pom.xmlthat inherits fromcom.google.cloud:sdk-platform-java config:3.62.0, aligninggrpc-gcpwith the rest of the google-cloud-java tooling (shared dependency BOM, release profile, formatter, deploy plugin).This is to migrate the repo to google-cloud-java, later we will switch to use Kokoro and Github actions once migrated for unit/integration tests
b/510577190