[controller] Add generic controller plugin framework by sushantmane · Pull Request #2668 · linkedin/venice

sushantmane · 2026-03-27T17:18:02Z

Summary

Introduce ControllerPlugin interface and ControllerPluginFactory for pluggable parent controller services
Support two plugin registration paths: programmatic (factory list on VeniceControllerContext) and reflection-based (comma-separated class names in controller.plugin.class.names config)
Plugins receive VeniceParentHelixAdmin, AuthorizerService, and VeniceControllerMultiClusterConfig for bootstrapping
Plugins are started after controller services and stopped during shutdown

Initial use case: wildcard ACL maintenance service (implementation in livenice PR).

Test plan

./gradlew :services:venice-controller:compileJava passes
CI passes
Companion livenice PR with plugin implementation and unit tests

Introduce a pluggable service mechanism for parent Venice controllers. External projects (e.g., livenice) can register plugins via two paths: 1. Programmatic: Set ControllerPluginFactory list on VeniceControllerContext 2. Reflection: Specify class names in controller.plugin.class.names config Each plugin implements ControllerPlugin (start/close/getName) and receives VeniceParentHelixAdmin, AuthorizerService, and VeniceControllerMultiClusterConfig for bootstrapping. Plugins are started after controller services and stopped during shutdown. Initial use case: wildcard ACL maintenance service (implementation in livenice).

Copilot

Pull request overview

Adds a generic “controller plugin” framework to allow pluggable services to be created and lifecycle-managed by the parent Venice controller.

Changes:

Introduces ControllerPlugin and ControllerPluginFactory extension points.
Adds plugin registration via VeniceControllerContext (programmatic factories) and via config (controller.plugin.class.names) using reflection.
Hooks plugin start/stop into VeniceController lifecycle.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerContext.java	Adds builder wiring for supplying plugin factories to the controller.
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceController.java	Creates plugins (factory + reflection paths) and starts/closes them with controller lifecycle.
services/venice-controller/src/main/java/com/linkedin/venice/controller/ControllerPluginFactory.java	Defines factory API for creating plugins with controller-provided dependencies.
services/venice-controller/src/main/java/com/linkedin/venice/controller/ControllerPlugin.java	Defines plugin lifecycle interface (`start`, `close`, `getName`).
internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java	Adds config key for reflection-based plugin class registration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-27T17:22:27Z

services/venice-controller/src/main/java/com/linkedin/venice/controller/ControllerPlugin.java

+/**
+ * A pluggable service that runs on the parent Venice controller.
+ * Implementations are provided externally (e.g., by downstream projects) and registered
+ * via {@link VeniceControllerContext.Builder#setControllerPluginFactories}.
+ *
+ * <p>The lifecycle is:
+ * <ol>
+ *   <li>{@link ControllerPluginFactory#create} — called during controller construction</li>
+ *   <li>{@link #start()} — called when the controller starts</li>
+ *   <li>{@link #close()} — called when the controller stops</li>
+ * </ol>


ControllerPlugin Javadoc only mentions registration via VeniceControllerContext.Builder#setControllerPluginFactories, but this PR also adds a config-based registration path (controller.plugin.class.names). Please update the interface documentation to reflect both supported registration mechanisms so downstream implementers know about the reflection option.

Copilot · 2026-03-27T17:22:27Z

internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java

+   * Comma-separated list of {@link com.linkedin.venice.controller.ControllerPlugin} implementation class names.
+   * Each class must have a public constructor taking
+   * ({@code VeniceParentHelixAdmin}, {@code AuthorizerService}, {@code VeniceControllerMultiClusterConfig}).
+   * Plugins are instantiated via reflection during parent controller startup.


Config Javadoc says plugins are instantiated "during parent controller startup", but the implementation instantiates them during VeniceController construction and starts them later in start(). Please align this comment with the actual lifecycle to avoid misleading operators and plugin authors.

Suggested change

* Plugins are instantiated via reflection during parent controller startup.

* Plugins are instantiated via reflection during {@code VeniceController} construction and started later when

* the controller's {@code start()} method is invoked.

Copilot · 2026-03-27T17:22:27Z

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceController.java

    deferredVersionSwapService.ifPresent(AbstractVeniceService::start);
+    for (ControllerPlugin plugin: controllerPlugins) {
+      LOGGER.info("Starting controller plugin: {}", plugin.getName());
+      plugin.start();


plugin.start() is invoked without any exception handling. Since plugin creation errors are already swallowed (logged) earlier, a runtime exception here would abort controller startup and potentially leave partially-started services running. Consider wrapping each plugin start in try/catch (and logging plugin name/class) so a faulty optional plugin can’t prevent the controller from starting.

Suggested change

plugin.start();

try {

plugin.start();

} catch (Exception e) {

LOGGER.error(

"Failed to start controller plugin: {} (class: {}). This optional plugin will be disabled.",

plugin.getName(),

plugin.getClass().getName(),

e);

}

Copilot · 2026-03-27T17:22:28Z

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceController.java

+          Class<? extends ControllerPlugin> pluginClass = ReflectUtils.loadClass(className);
+          ControllerPlugin plugin = ReflectUtils.callConstructor(
+              pluginClass,
+              new Class[] { VeniceParentHelixAdmin.class, AuthorizerService.class,
+                  VeniceControllerMultiClusterConfig.class },
+              new Object[] { parentAdmin, authService, multiClusterConfigs });
+          plugins.add(plugin);


For the reflection-based plugin path, the code relies on a ClassCastException later if the configured class doesn’t actually implement ControllerPlugin. Adding an explicit ControllerPlugin.class.isAssignableFrom(loadedClass) check would allow a clearer error message (and avoid attempting constructor invocation on unrelated classes).

Copilot · 2026-03-27T17:22:28Z

.../venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerContext.java

@@ -194,6 +201,11 @@ public Builder setExternalETLService(ExternalETLService externalETLService) {
      return this;
    }

+    public Builder setControllerPluginFactories(List<ControllerPluginFactory> controllerPluginFactories) {
+      this.controllerPluginFactories = controllerPluginFactories;
+      return this;
+    }


VeniceControllerContext adds controllerPluginFactories with a default and builder setter, but the existing VeniceControllerContextTest doesn’t assert the new default behavior or that setControllerPluginFactories(...) is wired through. Please extend the test coverage to prevent regressions (e.g., default should be an empty list, and the getter should return the value provided to the builder).

Copilot · 2026-03-27T17:22:28Z

.../venice-controller/src/main/java/com/linkedin/venice/controller/ControllerPluginFactory.java

+public interface ControllerPluginFactory {
+  ControllerPlugin create(
+      VeniceParentHelixAdmin admin,
+      AuthorizerService authorizerService,
+      VeniceControllerMultiClusterConfig config);


AuthorizerService is optional in VeniceControllerContext/VeniceController (it may be null), but the plugin API requires a non-null AuthorizerService. Please make the contract explicit (e.g., accept Optional<AuthorizerService> or clearly document/annotate that the value may be null) so plugin implementations don’t NPE when auth is disabled.

ThePatelCode · 2026-03-28T00:00:33Z

🤖 Code Review Summary

This PR introduces a clean and well-documented controller plugin framework with two registration paths (programmatic and reflection-based). The interfaces are well-defined and the lifecycle management (create → start → close) is straightforward. However, the error handling strategy is inconsistent between plugin creation and plugin startup.

Files reviewed: 5
Issues found: 2

[high][bug] plugin.start() in the startup loop has no exception handling — a failing plugin will abort the entire controller startup, preventing service discovery and gRPC servers from starting. This is inconsistent with the defensive error handling in createControllerPlugins(). (claude and codex review)
[medium][logic_error] Plugin creation failures in both the programmatic factory path and the reflection-based path are silently caught and logged at ERROR level. For the reflection path especially, where an operator explicitly configured a class name, silent failure means the controller appears healthy while the intended plugin is not running. Consider failing fast or emitting a metric. (codex review)

Reviewed by Claude Code and Codex

ThePatelCode · 2026-03-28T00:00:46Z

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceController.java

    deferredVersionSwapService.ifPresent(AbstractVeniceService::start);
+    for (ControllerPlugin plugin: controllerPlugins) {
+      LOGGER.info("Starting controller plugin: {}", plugin.getName());
+      plugin.start();


[high][bug] plugin.start() is called without any exception handling. If any plugin's start() throws, the entire controller startup is aborted — subsequent plugins won't start, service discovery registration will be skipped, and gRPC servers won't start. This is inconsistent with the defensive error handling in createControllerPlugins() (which catches exceptions and continues).

Since plugins are optional external extensions, a failing plugin should not take down the controller.

Suggestion:

for (ControllerPlugin plugin: controllerPlugins) { try { LOGGER.info("Starting controller plugin: {}", plugin.getName()); plugin.start(); } catch (Exception e) { LOGGER.error("Failed to start controller plugin: {}", plugin.getName(), e); } }

(claude and codex review)

ThePatelCode · 2026-03-28T00:00:46Z

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceController.java

+            plugins.add(plugin);
+            LOGGER.info("Created controller plugin from factory: {}", plugin.getName());
+          }
+        } catch (Exception e) {


[medium][logic_error] Plugin creation failures are silently caught and logged at ERROR level in both paths. For the reflection-based path (Path 2), where an operator has explicitly configured a class name in config, silent failure means the controller appears healthy while the intended plugin is not running. Consider logging at WARN level with a metric, or failing fast for the reflection path where operator intent is explicit.

(codex review)

Copilot AI review requested due to automatic review settings March 27, 2026 17:18

Copilot started reviewing on behalf of sushantmane March 27, 2026 17:18 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

ThePatelCode reviewed Mar 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[controller] Add generic controller plugin framework#2668

[controller] Add generic controller plugin framework#2668
sushantmane wants to merge 1 commit intolinkedin:mainfrom
sushantmane:wildcard-acl-maintenance

sushantmane commented Mar 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

ThePatelCode commented Mar 28, 2026

Uh oh!

ThePatelCode Mar 28, 2026

Uh oh!

ThePatelCode Mar 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	* Plugins are instantiated via reflection during parent controller startup.
	* Plugins are instantiated via reflection during {@code VeniceController} construction and started later when
	* the controller's {@code start()} method is invoked.

-      plugin.start();
+      try {
+        plugin.start();
+      } catch (Exception e) {
+        LOGGER.error(
+            "Failed to start controller plugin: {} (class: {}). This optional plugin will be disabled.",
+            plugin.getName(),
+            plugin.getClass().getName(),
+            e);
+      }

Conversation

sushantmane commented Mar 27, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

ThePatelCode commented Mar 28, 2026

🤖 Code Review Summary

Uh oh!

ThePatelCode Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

ThePatelCode Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants