[codex] Upgrade protobuf to 4.34.0#17995
Conversation
There was a problem hiding this comment.
Pull request overview
Upgrades Apache Pinot’s shared Protocol Buffers dependency to the 4.x line and fixes a build break in the pinot-confluent-protobuf module by making its javax.annotation (JSR-305) annotations dependency explicit.
Changes:
- Bump root
protobuf.versionfrom3.25.9to4.34.0(affecting protobuf BOM / protoc usage across the build). - Add an explicit
com.google.code.findbugs:jsr305dependency topinot-confluent-protobufto restore access tojavax.annotation.Nullable/ParametersAreNonnullByDefaultunder protobuf 4.x.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pom.xml |
Updates the shared protobuf version property used by protobuf BOM and protoc artifact resolution. |
pinot-plugins/pinot-input-format/pinot-confluent-protobuf/pom.xml |
Declares jsr305 directly so the module compiles without relying on removed transitive annotations. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17995 +/- ##
============================================
- Coverage 63.37% 63.27% -0.10%
Complexity 1543 1543
============================================
Files 3200 3200
Lines 194169 194169
Branches 29915 29915
============================================
- Hits 123051 122867 -184
- Misses 61466 61650 +184
Partials 9652 9652
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0968adb to
fbde5ec
Compare
pinot-plugins/pinot-input-format/pinot-confluent-protobuf/pom.xml
Outdated
Show resolved
Hide resolved
pinot-plugins/pinot-input-format/pinot-confluent-protobuf/pom.xml
Outdated
Show resolved
Hide resolved
pinot-plugins/pinot-input-format/pinot-confluent-protobuf/pom.xml
Outdated
Show resolved
Hide resolved
3a3fda2 to
a0964fa
Compare
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Can you try also bumping google cloud version? I think it should also be bumped after upgrading protobuf
...va/org/apache/pinot/controller/helix/core/minion/PinotTaskManagerDistributedLockingTest.java
Outdated
Show resolved
Hide resolved
| <groupId>io.confluent</groupId> | ||
| <artifactId>kafka-schema-registry-client</artifactId> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Yes, it is necessary. The module directly uses @Nullable from javax.annotation in KafkaConfluentSchemaRegistryProtoBufMessageDecoder.java (4 usages). Without it, compilation fails with 5 errors. After protobuf 4.x, protobuf-java no longer transitively pulls in jsr305 at compile scope.
That said, I've added <scope>provided</scope> since it's only needed at compile time for annotations and shouldn't be bundled into the shaded plugin JAR.
There was a problem hiding this comment.
Should we just include it in pinot-spi? This is a very basic dependency which should be included in all modules
There was a problem hiding this comment.
jsr305 is already transitively available to all core modules via Guava in pinot-spi (guava -> jsr305:3.0.2:compile). The reason pinot-confluent-protobuf needs it explicitly is that plugin modules declare pinot-spi with provided scope, which makes its transitive dependencies (including jsr305 via Guava) unavailable at compile time.
So adding jsr305 to pinot-spi explicitly wouldn't change anything for plugins — they'd still need their own declaration. The current approach (explicit jsr305 with provided scope in this plugin module) is the minimal correct fix.
- Bump google.cloud.libraries.version from 26.74.0 to 26.79.0 to align with protobuf 4.x upgrade - Add provided scope to jsr305 dependency in pinot-confluent-protobuf since it is only needed at compile time for javax.annotation annotations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8f64c7a to
9c0b140
Compare
|
Bumped |
* Upgrade protobuf to 4.34.0 * Scope Confluent protobuf plugin to protobuf 3.x * Align Confluent protobuf plugin with shared protobuf version * Stabilize protobuf upgrade CI * Upgrade Confluent schema registry to 8.2.0 * Drop unrelated controller test tweak * Fix protobuf upgrade CI regressions * Revert sqlparser CI script changes * Address PR review comments: bump Google Cloud libs, scope jsr305 - Bump google.cloud.libraries.version from 26.74.0 to 26.79.0 to align with protobuf 4.x upgrade - Add provided scope to jsr305 dependency in pinot-confluent-protobuf since it is only needed at compile time for javax.annotation annotations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
protobuf.versionto4.34.0.confluent.versionto8.2.0, which publishes protobuf 4.x artifacts.com.google.code.findbugs:jsr305dependency topinot-confluent-protobufso the module still getsjavax.annotationtypes after the shared protobuf upgrade.Why
Pinot should move its shared protobuf dependency to 4.x without leaving the Confluent input plugins on an older serializer stack. Confluent 8.2.0 now aligns with protobuf 4.x, so the repo can move both versions forward together instead of carrying a plugin-local protobuf 3.x carve-out.
The version bump also surfaced one direct source-level compatibility issue in tests: Confluent 8.2 removed
AbstractKafkaAvroSerDeConfig, so Pinots Avro schema-registry integration test needs to useAbstractKafkaSchemaSerDeConfiginstead.User / Developer Impact
pinot-confluent-protobufstays on the shared protobuf 4.x line instead of maintaining a separate protobuf 3.x island.pinot-confluent-protobufno longer relies on transitive resolution forjavax.annotationtypes.Root Cause
pinot-confluent-protobufreferencedjavax.annotation.Nullableandjavax.annotation.ParametersAreNonnullByDefaultwithout declaring JSR-305 directly.io.confluent.kafka.serializers.AbstractKafkaAvroSerDeConfig, but Pinot integration test code still imported it.How To Reproduce
master.protobuf.versionto a protobuf 4.x release and the sharedconfluent.versionto8.2.0../mvnw -pl pinot-plugins/pinot-input-format/pinot-confluent-protobuf -am -DskipITs -Dskip.integration.tests=true -DskipTests compile.javax.annotationcompilation failures unlessjsr305is declared explicitly.pinot-integration-testsand observe the staleAbstractKafkaAvroSerDeConfigimport fail against Confluent 8.2.Validation
./mvnw -pl pinot-integration-tests spotless:apply./mvnw -pl pinot-integration-tests checkstyle:check./mvnw -pl pinot-integration-tests license:format./mvnw -pl pinot-integration-tests license:check./mvnw -pl pinot-common,pinot-server,pinot-query-planner,pinot-query-runtime -am -DskipITs -Dskip.integration.tests=true -DskipTests compile./mvnw -pl pinot-plugins/pinot-input-format/pinot-confluent-protobuf -am -DskipITs -Dskip.integration.tests=true -DskipTests compile./mvnw -pl pinot-plugins/pinot-input-format/pinot-confluent-avro,pinot-plugins/pinot-input-format/pinot-confluent-json,pinot-plugins/pinot-input-format/pinot-confluent-protobuf -am -DskipITs -Dskip.integration.tests=true -DskipTests compileKnown Test Limitation