Replace global shading with classloader-realm isolation for plugins

CLAUDE.md

@@ -49,8 +49,10 @@ Apache Pinot is a real-time distributed OLAP datastore for low-latency analytics
   - `pinot-confluent-json`: Confluent Schema Registry JSON input support.
   - `pinot-confluent-protobuf`: Confluent Schema Registry Protobuf input support.
   - `pinot-orc`: ORC input format support.
-  - `pinot-json`: JSON input format support.
-  - `pinot-parquet`: Parquet input format support.
+  - `pinot-json-base`: shared JSON utilities and base classes (JSONRecordReader, JSONRecordExtractor).
+  - `pinot-json`: JSON plugin (JSONMessageDecoder); depends on pinot-json-base.
+  - `pinot-parquet-base`: shared Parquet utilities (ParquetUtils); depends on parquet-avro and hadoop.
+  - `pinot-parquet`: Parquet plugin (ParquetRecordReader and variants); depends on pinot-parquet-base.
   - `pinot-csv`: CSV input format support.
   - `pinot-thrift`: Thrift input format support.
   - `pinot-protobuf`: Protobuf input format support.

pinot-broker/src/main/java/org/apache/pinot/broker/broker/AccessControlFactory.java

@@ -28,6 +28,7 @@
 import org.apache.pinot.broker.grpc.GrpcRequesterIdentity;
 import org.apache.pinot.spi.auth.broker.RequesterIdentity;
 import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.plugin.PluginManager;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -62,8 +63,16 @@ public static AccessControlFactory loadFactory(PinotConfiguration configuration,
     }
     try {
       LOGGER.info("Instantiating Access control factory class {}", accessControlFactoryClassName);
-      accessControlFactory =
-          (AccessControlFactory) Class.forName(accessControlFactoryClassName).getDeclaredConstructor().newInstance();
+      // PluginManager.loadClass is realm-aware (consults plugin realms in addition to the
+      // system classloader). We deliberately keep the explicit getDeclaredConstructor() +
+      // newInstance() pattern instead of going through PluginManager.createInstance, because
+      // the former preserves the historical contract that factories with package-private
+      // no-arg constructors load successfully (PluginManager.createInstance uses
+      // getConstructor() which only finds public ones). Truly-private constructors still
+      // require setAccessible(true), which we do not do — match the prior Class.forName
+      // behaviour exactly.
+      Class<?> clazz = PluginManager.get().loadClass(accessControlFactoryClassName);
+      accessControlFactory = (AccessControlFactory) clazz.getDeclaredConstructor().newInstance();
       LOGGER.info("Initializing Access control factory class {}", accessControlFactoryClassName);
       accessControlFactory.init(configuration, propertyStore);
       return accessControlFactory;

pinot-broker/src/test/java/org/apache/pinot/broker/broker/AccessControlFactoryLoaderTest.java

@@ -0,0 +1,95 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.broker.broker;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
+
+
+/**
+ * Regression tests for {@link AccessControlFactory#loadFactory(org.apache.pinot.spi.env.PinotConfiguration,
+ * org.apache.helix.store.zk.ZkHelixPropertyStore)}. The loadFactory resolution is realm-aware via
+ * PluginManager.loadClass, so plugin-realm-resident factories work the same as in-tree ones — but the
+ * non-public-constructor contract from the historical Class.forName(...).getDeclaredConstructor()
+ * pattern must be preserved.
+ */
+public class AccessControlFactoryLoaderTest {
+
+  @Test
+  public void loadFactoryWithNullClassConfigReturnsAllowAllAccessControlFactory() {
+    PinotConfiguration emptyConfig = new PinotConfiguration(Collections.emptyMap());
+    AccessControlFactory factory = AccessControlFactory.loadFactory(emptyConfig, null);
+    assertTrue(factory instanceof AllowAllAccessControlFactory,
+        "Expected AllowAllAccessControlFactory when no `class` config is set, got: " + factory.getClass());
+  }
+
+  @Test
+  public void loadFactoryWithExplicitInTreeFqcnResolvesViaSystemClassloader() {
+    Map<String, Object> props = new HashMap<>();
+    props.put(AccessControlFactory.ACCESS_CONTROL_CLASS_CONFIG, AllowAllAccessControlFactory.class.getName());
+    AccessControlFactory factory = AccessControlFactory.loadFactory(new PinotConfiguration(props), null);
+    assertEquals(factory.getClass(), AllowAllAccessControlFactory.class);
+  }
+
+  @Test
+  public void loadFactoryWithUnknownClassThrowsRuntimeExceptionWrappingClassNotFound() {
+    Map<String, Object> props = new HashMap<>();
+    props.put(AccessControlFactory.ACCESS_CONTROL_CLASS_CONFIG, "org.apache.pinot.does.not.Exist");
+    try {
+      AccessControlFactory.loadFactory(new PinotConfiguration(props), null);
+      fail("Expected RuntimeException for unknown class");
+    } catch (RuntimeException thrown) {
+      Throwable cause = thrown.getCause();
+      assertTrue(cause instanceof ClassNotFoundException || cause instanceof NoClassDefFoundError
+              || (cause != null && cause.getCause() instanceof ClassNotFoundException),
+          "Expected ClassNotFoundException as the root cause, got: " + cause);
+    }
+  }
+
+  /**
+   * Regression test: the historical loader used getDeclaredConstructor() which finds non-public no-arg
+   * constructors. The realm-aware migration must preserve that contract or factories with
+   * package-private / protected constructors would silently break on upgrade.
+   */
+  @Test
+  public void loadFactoryWithPackagePrivateConstructorSucceeds() {
+    Map<String, Object> props = new HashMap<>();
+    props.put(AccessControlFactory.ACCESS_CONTROL_CLASS_CONFIG, PackagePrivateConstructorFactory.class.getName());
+    AccessControlFactory factory = AccessControlFactory.loadFactory(new PinotConfiguration(props), null);
+    assertEquals(factory.getClass(), PackagePrivateConstructorFactory.class);
+  }
+
+  /** Test fixture: factory whose no-arg constructor is package-private. */
+  public static class PackagePrivateConstructorFactory extends AccessControlFactory {
+    PackagePrivateConstructorFactory() {
+    }
+
+    @Override
+    public org.apache.pinot.broker.api.AccessControl create() {
+      throw new UnsupportedOperationException("test fixture");
+    }
+  }
+}

pinot-clients/pinot-java-client/pom.xml

@@ -53,6 +53,28 @@
           <target>11</target>
         </configuration>
       </plugin>
+      <!-- Driver jar: relocate bundled jackson/guava/scala so they don't leak
+           onto the embedding application's classpath. -->
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-shade-plugin</artifactId>
+        <configuration>
+          <relocations>
+            <relocation>
+              <pattern>com.fasterxml.jackson</pattern>
+              <shadedPattern>${shade.prefix}.com.fasterxml.jackson</shadedPattern>
+            </relocation>
+            <relocation>
+              <pattern>com.google.common</pattern>
+              <shadedPattern>${shade.prefix}.com.google.common</shadedPattern>
+            </relocation>
+            <relocation>
+              <pattern>org.scala-lang</pattern>
+              <shadedPattern>${shade.prefix}.org.scala-lang</shadedPattern>
+            </relocation>
+          </relocations>
+        </configuration>
+      </plugin>
     </plugins>
   </build>
   <dependencies>

pinot-clients/pinot-jdbc-client/pom.xml

@@ -31,6 +31,7 @@
   <url>https://pinot.apache.org/</url>
   <properties>
     <pinot.root>${basedir}/../..</pinot.root>
+    <shade.phase.prop>package</shade.phase.prop>
   </properties>
   <build>
     <resources>
@@ -59,6 +60,28 @@
           <target>11</target>
         </configuration>
       </plugin>
+      <!-- Driver jar: relocate bundled jackson/guava/scala so they don't leak
+           onto the embedding application's classpath. -->
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-shade-plugin</artifactId>
+        <configuration>
+          <relocations>
+            <relocation>
+              <pattern>com.fasterxml.jackson</pattern>
+              <shadedPattern>${shade.prefix}.com.fasterxml.jackson</shadedPattern>
+            </relocation>
+            <relocation>
+              <pattern>com.google.common</pattern>
+              <shadedPattern>${shade.prefix}.com.google.common</shadedPattern>
+            </relocation>
+            <relocation>
+              <pattern>org.scala-lang</pattern>
+              <shadedPattern>${shade.prefix}.org.scala-lang</shadedPattern>
+            </relocation>
+          </relocations>
+        </configuration>
+      </plugin>
     </plugins>
   </build>
   <dependencies>
@@ -76,18 +99,6 @@
     </dependency>
   </dependencies>
   <profiles>
-    <profile>
-      <id>build-shaded-jar</id>
-      <activation>
-        <property>
-          <name>skipShade</name>
-          <value>!true</value>
-        </property>
-      </activation>
-      <properties>
-        <shade.phase.prop>package</shade.phase.prop>
-      </properties>
-    </profile>
     <profile>
       <id>pinot-fastdev</id>
       <properties>

pinot-common/pom.xml

@@ -369,15 +369,6 @@
       </build>
     </profile>
 
-    <profile>
-      <id>build-shaded-jar</id>
-      <activation>
-        <activeByDefault>true</activeByDefault>
-      </activation>
-      <properties>
-        <shade.phase.prop>package</shade.phase.prop>
-      </properties>
-    </profile>
     <profile>
       <id>pinot-fastdev</id>
       <properties>

pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigSerDeProvider.java

@@ -19,13 +19,13 @@
 package org.apache.pinot.common.utils;
 
 import com.google.common.annotations.VisibleForTesting;
-import java.util.ServiceLoader;
+import org.apache.pinot.spi.plugin.PluginManager;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 
 /**
- * Singleton holder for {@link LogicalTableConfigSerDe}, loaded via {@link ServiceLoader}.
+ * Singleton holder for {@link LogicalTableConfigSerDe}, loaded via {@link PluginManager#loadServices(Class)}.
  *
  * <p>When multiple implementations are discovered, the one with the highest
  * {@link LogicalTableConfigSerDe#getPriority()} is selected</p>
@@ -54,7 +54,7 @@ private static LogicalTableConfigSerDe fromServiceLoader() {
     LogicalTableConfigSerDe best = null;
     int bestPriority = Integer.MIN_VALUE;
 
-    for (LogicalTableConfigSerDe serDe : ServiceLoader.load(LogicalTableConfigSerDe.class)) {
+    for (LogicalTableConfigSerDe serDe : PluginManager.get().loadServices(LogicalTableConfigSerDe.class)) {
       LOGGER.info("Discovered LogicalTableConfigSerDe: {} with priority {}",
           serDe.getClass().getName(), serDe.getPriority());
       if (serDe.getPriority() > bestPriority) {

pinot-common/src/test/java/org/apache/pinot/plugin/metrics/fake/FakeMetricsFactory.java

@@ -18,6 +18,7 @@
  */
 package org.apache.pinot.plugin.metrics.fake;
 
+import com.google.auto.service.AutoService;
 import java.util.function.Function;
 import org.apache.pinot.spi.annotations.metrics.MetricsFactory;
 import org.apache.pinot.spi.annotations.metrics.PinotMetricsFactory;
@@ -31,7 +32,13 @@
 /**
  * A test-only {@link PinotMetricsFactory} backed by the in-memory {@link FakePinotMetricsRegistry}. Used by
  * pinot-common tests so they can exercise {@code AbstractMetrics} without depending on a plugin module.
+ *
+ * <p>Carries both {@code @AutoService(PinotMetricsFactory.class)} (generates the
+ * {@code META-INF/services} entry consumed by {@code PinotMetricUtils} via
+ * {@code PluginManager.loadServices}) and the legacy {@code @MetricsFactory} annotation which the
+ * factory loader still inspects to honour {@code enabled = false}.</p>
  */
+@AutoService(PinotMetricsFactory.class)
 @MetricsFactory
 public class FakeMetricsFactory implements PinotMetricsFactory {
   private PinotMetricsRegistry _registry;

pinot-core/pom.xml

@@ -173,15 +173,6 @@
   </dependencies>
 
   <profiles>
-    <profile>
-      <id>build-shaded-jar</id>
-      <activation>
-        <activeByDefault>false</activeByDefault>
-      </activation>
-      <properties>
-        <shade.phase.prop>package</shade.phase.prop>
-      </properties>
-    </profile>
     <profile>
       <id>pinot-fastdev</id>
       <properties>

pinot-core/src/main/java/org/apache/pinot/core/routing/timeboundary/TimeBoundaryStrategyService.java

@@ -19,7 +19,7 @@
 package org.apache.pinot.core.routing.timeboundary;
 
 import java.util.Map;
-import java.util.ServiceLoader;
+import org.apache.pinot.spi.plugin.PluginManager;
 
 
 public class TimeBoundaryStrategyService {
@@ -33,7 +33,7 @@ private TimeBoundaryStrategyService(Map<String, TimeBoundaryStrategy> strategyMa
 
   public static TimeBoundaryStrategyService fromServiceLoader() {
     Map<String, TimeBoundaryStrategy> strategyMap = new java.util.HashMap<>();
-    for (TimeBoundaryStrategy strategy : ServiceLoader.load(TimeBoundaryStrategy.class)) {
+    for (TimeBoundaryStrategy strategy : PluginManager.get().loadServices(TimeBoundaryStrategy.class)) {
       String strategyName = strategy.getName();
       if (strategyMap.containsKey(strategyName)) {
         throw new IllegalStateException("Duplicate TimeBoundaryStrategy found: " + strategyName);