Copilot commented on code in PR #17995:
URL: https://github.com/apache/pinot/pull/17995#discussion_r3003528195
##########
pom.xml:
##########
@@ -199,7 +199,7 @@
<spark3.version>3.5.8</spark3.version>
<kafka3.version>3.9.2</kafka3.version>
<kafka4.version>4.1.1</kafka4.version>
- <confluent.version>7.9.5</confluent.version>
+ <confluent.version>8.2.0</confluent.version>
Review Comment:
This PR changes `confluent.version` (7.9.5 -> 8.2.0), but the PR
title/description only describe the protobuf upgrade. Please either document
why the Confluent BOM/version bump is required here (and any compatibility
implications), or split it into a separate PR to keep the change scoped.
##########
pinot-plugins/pinot-input-format/pinot-confluent-protobuf/pom.xml:
##########
@@ -53,6 +53,10 @@
<groupId>io.confluent</groupId>
<artifactId>kafka-schema-registry-client</artifactId>
</dependency>
+ <dependency>
+ <groupId>com.google.code.findbugs</groupId>
+ <artifactId>jsr305</artifactId>
Review Comment:
`jsr305` appears to be added solely to provide `javax.annotation.Nullable`
(annotation-only usage). Consider declaring it with `provided` scope (or
otherwise ensuring it isn’t unnecessarily bundled into the shaded plugin JAR),
since the annotations aren’t referenced directly at runtime and this avoids
shipping duplicate `javax.annotation` classes in the plugin artifact.
```suggestion
<artifactId>jsr305</artifactId>
<scope>provided</scope>
```
##########
pom.xml:
##########
@@ -240,7 +240,7 @@
<httpcore5.version>5.4.2</httpcore5.version>
<!-- Google Libraries -->
- <protobuf.version>3.25.9</protobuf.version>
+ <protobuf.version>4.34.0</protobuf.version>
Review Comment:
The PR description says `pinot-confluent-protobuf` must be isolated because
Pinot plugin classloading is parent-first, and that the plugin’s protobuf
classes will be shaded. However, the root `maven-shade-plugin` configuration
does not relocate `com.google.protobuf` (only Jackson/Guava/Scala are
relocated), so packaging a plugin-local protobuf runtime under the original
package name won’t reliably isolate it from the system protobuf 4.x classes. To
make the isolation work, add an explicit relocation for `com.google.protobuf`
(and any other protobuf packages used) for the plugin(s) that need it, or
introduce a module-specific shade configuration that performs this relocation.
##########
pinot-plugins/pinot-input-format/pinot-confluent-protobuf/pom.xml:
##########
@@ -53,6 +53,10 @@
<groupId>io.confluent</groupId>
<artifactId>kafka-schema-registry-client</artifactId>
</dependency>
+ <dependency>
+ <groupId>com.google.code.findbugs</groupId>
+ <artifactId>jsr305</artifactId>
+ </dependency>
Review Comment:
`protobuf-maven-plugin` is configured in the root POM to use
`protoc:${protobuf.version}`. After this PR bumps `protobuf.version` to 4.34.0,
this module will also generate its test protos with protoc 4.34.0 because it
doesn’t override `protocArtifact`. That conflicts with the PR description’s
claim that this module’s test protos are generated with protoc 3.25.9; please
override the `protocArtifact` (and any related protobuf runtime version
alignment) in this module so the build matches the intended 3.x Confluent
dependency path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
