Copilot commented on code in PR #17995:
URL: https://github.com/apache/pinot/pull/17995#discussion_r2998761947
##########
pinot-plugins/pinot-input-format/pinot-confluent-protobuf/pom.xml:
##########
@@ -44,15 +45,51 @@
<dependency>
<groupId>org.apache.pinot</groupId>
<artifactId>pinot-protobuf</artifactId>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java</artifactId>
+ </exclusion>
+ </exclusions>
</dependency>
<dependency>
<groupId>io.confluent</groupId>
<artifactId>kafka-protobuf-serializer</artifactId>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java</artifactId>
+ </exclusion>
+ <exclusion>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java-util</artifactId>
+ </exclusion>
+ </exclusions>
</dependency>
<dependency>
<groupId>io.confluent</groupId>
<artifactId>kafka-schema-registry-client</artifactId>
</dependency>
+ <dependency>
+ <groupId>com.google.code.findbugs</groupId>
+ <artifactId>jsr305</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java</artifactId>
+ <version>${pinot.confluent.protobuf.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java-util</artifactId>
+ <version>${pinot.confluent.protobuf.version}</version>
+ <exclusions>
Review Comment:
`pinot-confluent-protobuf` is now pinning protobuf artifacts to 3.25.9 (via
`${pinot.confluent.protobuf.version}` + exclusions + explicit
`protobuf-java`/`protobuf-java-util` versions). Pinot’s plugin loading is
parent-first (see
`pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginClassLoader.java`,
`loadClass()` tries the system classloader before `findClass`), and
`pinot-server`/`pinot-common` include `com.google.protobuf:protobuf-java` on
the system classpath. That means this plugin will still resolve protobuf from
the system (now 4.34.0) at runtime, so the 3.25.9 pin is unlikely to take
effect and can lead to runtime linkage errors if Confluent components are not
protobuf-4-compatible. Prefer aligning this module to the shared
`${protobuf.version}` (remove the protobuf exclusions/explicit versions), or
alternatively relocate/shade protobuf for this plugin so it cannot conflict
with the system classpath.
##########
pinot-plugins/pinot-input-format/pinot-confluent-protobuf/pom.xml:
##########
@@ -71,6 +108,9 @@
<groupId>org.xolstice.maven.plugins</groupId>
<artifactId>protobuf-maven-plugin</artifactId>
<configuration>
+ <protocArtifact>
+
com.google.protobuf:protoc:${pinot.confluent.protobuf.version}:exe:${os.detected.classifier}
Review Comment:
The `protobuf-maven-plugin` configuration overrides `protocArtifact` to use
`${pinot.confluent.protobuf.version}` (3.25.9). If the rest of the build is
moving to protobuf 4.x, generating test protos with a different major-version
`protoc` increases the chance of subtle incompatibilities (especially around
proto3 optional / descriptor behavior that this module’s tests validate).
Consider using the shared `${protobuf.version}` for `protocArtifact` as well,
or document/justify why this module must generate with protoc 3.x while the
runtime is 4.x.
```suggestion
com.google.protobuf:protoc:${protobuf.version}:exe:${os.detected.classifier}
```
--
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]
