xiangfu0 commented on code in PR #20112:
URL: https://github.com/apache/pinot/pull/20112#discussion_r4434090663
##########
pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java:
##########
@@ -526,6 +533,26 @@ public void registerRecordReaderClass(String inputFormat,
String recordReaderCla
}
}
+ private String getBackwardCompatiblePluginName(String pluginName) {
+ return _backwardCompatiblePluginNames.getOrDefault(pluginName, pluginName);
+ }
+
+ /// Registers a direct alias for a renamed plugin, including
`plugins.include` and plugin-qualified class names.
+ /// Call during bootstrap, before creating plugin instances. If eager
initialization already skipped the replacement
+ /// because only the old name was included, loads that replacement now.
Repeated registration is harmless.
+ public synchronized void registerBackwardCompatiblePluginName(String
oldPluginName, String newPluginName) {
Review Comment:
MINOR: This adds a public `pinot-spi` API that nothing in production calls.
It also writes to `_backwardCompatiblePluginNames`, a plain `HashMap`, while
`loadClass`/`createInstance` read that map without synchronization from
consumer threads. Suggest dropping the method and making the alias map an
immutable `Map.of(...)`. If you keep it, use a `ConcurrentHashMap`.
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5).
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/LaunchSparkDataIngestionJobCommand.java:
##########
@@ -71,7 +71,7 @@ public class LaunchSparkDataIngestionJobCommand extends
AbstractBaseAdminCommand
// Kafka plugins need to be excluded as they contain scala dependencies
which cause
// NoSuchMethodErrors with runtime spark.
// It is also fine to exclude Kafka plugins as they are not going to be used
in batch ingestion in any case
- @CommandLine.Option(names = {"-pluginsToExclude"}, defaultValue =
"pinot-kafka-3.0", required =
+ @CommandLine.Option(names = {"-pluginsToExclude"}, defaultValue =
"pinot-kafka-4.0", required =
Review Comment:
MINOR: `shouldLoadPlugin` compares directory names directly, so an existing
explicit `-pluginsToExclude pinot-kafka-3.0` (or `-pluginsToLoad`) is now a
silent no-op. The comment above about Scala dependencies also no longer applies
to the Kafka 4 jar. Consider applying the same `pinot-kafka-2.0/3.0` → `4.0`
alias here and updating the comment.
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5).
##########
pinot-controller/src/main/resources/app/components/Homepage/Operations/AddRealtimeTableOp.tsx:
##########
@@ -100,7 +100,7 @@ const defaultTableObj = {
"stream.kafka.topic.name": "",
"stream.kafka.broker.list": "",
"stream.kafka.consumer.prop.auto.offset.reset": "smallest",
- "stream.kafka.consumer.factory.class.name":
"org.apache.pinot.plugin.stream.kafka30.KafkaConsumerFactory",
+ "stream.kafka.consumer.factory.class.name":
"org.apache.pinot.plugin.stream.kafka40.KafkaConsumerFactory",
Review Comment:
MINOR: Controllers usually upgrade first. Once they do, this UI default
writes the `kafka40` factory name into new tables while servers may still be on
images without `pinot-kafka-4.0`. Those servers would then fail to consume,
which contradicts the README's rollout guidance. Suggest omitting the factory
key here and in `AddIngestionComponent.tsx` /
`AddRealTimeIngestionComponent.tsx`, so each component falls back to its own
`StreamConfig` default.
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5).
--
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]
