[Improvement]: Load process factories via DefaultTableRuntimeFactory by baiyangtx · Pull Request #4100 · apache/amoro

baiyangtx · 2026-02-26T08:12:28Z

Summary

This PR is a subtask of #4100

Wire the new process plugin model(Introduced by #4097) into AMS so that table processes are discovered from ProcessFactory implementations and scheduled via the existing ProcessService.

Details

Promote ProcessFactory to the single extension point for table processes.
- DefaultTableRuntimeFactory aggregates all ProcessFactory implementations, exposes derived ActionCoordinators, and merges their requiredStates() with DefaultTableRuntime.REQUIRED_STATES.
Simplify TableRuntimeFactory so it only describes table runtime & process wiring.
- It no longer extends ActivePlugin; lifecycle methods (open/close/name) are removed from the interface and from DefaultTableRuntimeFactory.
Wire AMS directly to DefaultTableRuntimeFactory instead of a generic plugin manager.
- AmoroServiceContainer creates a single DefaultTableRuntimeFactory, passes it to DefaultTableService, then initializes it with all ProcessFactory implementations and uses supportedCoordinators() to build the coordinator list for ProcessService.
- DefaultTableService now depends on a TableRuntimeFactory directly and uses it to create TableRuntime instances.
Clean up ProcessService constructors to match the new model.
- Remove the deprecated constructor using ActionCoordinatorManager.
- AMS uses the constructor with an explicit List<ActionCoordinator>; tests use the 2‑argument convenience overload.

## Summary Wire the new process plugin model into AMS so that table processes are discovered from `ProcessFactory` implementations and scheduled via the existing `ProcessService`. ## Details - Extend `DefaultTableRuntimeFactory` to implement the `TableRuntimeFactory` process APIs: - Aggregate installed `ProcessFactory` instances by `TableFormat` / `Action` and expose derived `ActionCoordinator` plugins via `supportedCoordinators()` - Merge `DefaultTableRuntime.REQUIRED_STATES` with additional states required by all process factories for the same format when building `TableRuntimeCreator` - Keep using `DefaultTableRuntime` without introducing extra runtime types - Introduce `DefaultActionCoordinator` to bridge `ProcessFactory` trigger/recover semantics to the existing scheduler: - Build trigger strategies from `ProcessFactory#triggerStrategy` - Delegate `trigger` and `recoverTableProcess` to the underlying factory - Add `TableProcessFactoryManager` as an `AbstractPluginManager<ProcessFactory>` using the `process-factories` plugin namespace - Refactor `AmoroServiceContainer` startup sequence: - Initialize `TableProcessFactoryManager` and collect all installed `ProcessFactory` instances - Initialize all `TableRuntimeFactory` plugins with the shared list of process factories - Collect all derived `ActionCoordinator`s from table runtime factories and inject them into `ProcessService` - Update `ProcessService` to accept a pre-built list of `ActionCoordinator`s while keeping the original constructors for backward compatibility and tests - Run `mvn spotless:apply` and `mvn -pl amoro-ams -am -DskipTests compile` to ensure style and compilation pass Co-Authored-By: Aime <aime@bytedance.com> Change-Id: Iaa867503c8b0bf93c2b7f0b8fe7d752e2ddbac67

…in and use default runtime ## Summary Decouple `TableRuntimeFactory` from the generic `ActivePlugin` mechanism and make `AmoroServiceContainer` explicitly use `DefaultTableRuntimeFactory` as the default implementation. ## Details - Change `TableRuntimeFactory` in `amoro-common` so it no longer extends `ActivePlugin`, keeping only process-related APIs: - `List<ActionCoordinator> supportedCoordinators()` - `void initialize(List<ProcessFactory> factories)` - `Optional<TableRuntimeCreator> accept(ServerTableIdentifier, Map<String, String>)` - Refactor `TableRuntimeFactoryManager` in AMS: - Remove inheritance from `AbstractPluginManager<TableRuntimeFactory>` to avoid the `ActivePlugin` constraint - Implement a simple manager that wraps a provided `List<TableRuntimeFactory>` - Provide a default no-arg constructor that installs a single `DefaultTableRuntimeFactory` - Keep `initialize()` as a no-op and `installedPlugins()` as the accessor to preserve existing wiring in `DefaultTableService` - Update `DefaultTableRuntimeFactory`: - Remove `@Override` annotations from `open/close/name` since they no longer implement `ActivePlugin` methods - Keep the methods as no-op helpers with the same behavior - Update `AmoroServiceContainer.startOptimizingService` to use `DefaultTableRuntimeFactory` directly: - Construct a `DefaultTableRuntimeFactory` instance explicitly - Wrap it into `TableRuntimeFactoryManager` via `Collections.singletonList(tableRuntimeFactory)` - Use the resulting singleton list as the only table runtime factory when initializing process factories and collecting `ActionCoordinator`s - Leave `DefaultTableService` logic unchanged, it still uses `TableRuntimeFactoryManager.installedPlugins()` but now operates over the explicit default factory list - Ensure `ProcessService` changes from previous step are included in this commit so the module compiles cleanly ## Verification - Ran `./mvnw -pl amoro-ams -am -DskipTests compile` from repo root - Build succeeded for the full AMS reactor - `spotless` and `checkstyle` passed with only existing warnings unrelated to this change Co-Authored-By: Aime <aime@bytedance.com> Change-Id: Ifa8deef0d2553176300cdef2c0cb073d52ee3303

… default factory ## Summary - Remove TableRuntimeFactoryManager indirection and wire DefaultTableService directly with a TableRuntimeFactory implementation. - Simplify DefaultTableRuntimeFactory after decoupling from plugin framework and ActivePlugin lifecycle. - Inline default table runtime factory wiring into AmoroServiceContainer and tests, and update process/service initialization. ## Details - DefaultTableService - Change constructor to accept a TableRuntimeFactory instead of TableRuntimeFactoryManager. - Replace iteration over installed factories with a single factory.accept(...) call when creating TableRuntime instances. - DefaultTableRuntimeFactory - Drop unused ActivePlugin-style methods: open(Map<String, String>), close(), and name(). - AmoroServiceContainer - Instantiate a single DefaultTableRuntimeFactory in startOptimizingService and pass it into DefaultTableService. - Initialize the defaultRuntimeFactory with ProcessFactory plugins and derive ActionCoordinators from it directly. - Tests - AMSServiceTestBase: construct DefaultTableService with a concrete DefaultTableRuntimeFactory instead of a mocked TableRuntimeFactoryManager. - TestDefaultTableRuntimeHandler: hold a DefaultTableRuntimeFactory field and pass it into DefaultTableService for all test setups. - Cleanup - Delete TableRuntimeFactoryManager class and remove all references and related imports across main and test code. - Fix spotless formatting violations in the touched files so that `mvn -pl amoro-ams -am -DskipTests compile` passes. Co-Authored-By: Aime <aime@bytedance.com> Change-Id: I8acca1841470dfc1bbd87b77e424a34f4d52ae82

zhoujinsong

The change looks good to me over all, I left some small suggestion, PTLA.

zhoujinsong · 2026-03-04T11:59:51Z

amoro-ams/src/main/java/org/apache/amoro/server/process/ProcessService.java

      new ConcurrentHashMap<>();
  private final Map<EngineType, ExecuteEngine> executeEngines = new ConcurrentHashMap<>();

-  private final ActionCoordinatorManager actionCoordinatorManager;


It seems the ActionCoordinatorManager class can be dropped now.

zhoujinsong · 2026-03-04T12:18:02Z

amoro-common/src/main/java/org/apache/amoro/table/TableRuntimeFactory.java


 /** Table runtime factory. */
-public interface TableRuntimeFactory extends ActivePlugin {
+public interface TableRuntimeFactory {


Do we still need other table runtime factory implementation now, or is the default the only one?

no, only this one

zhoujinsong · 2026-03-04T12:20:56Z

amoro-ams/src/main/java/org/apache/amoro/server/process/ProcessService.java

  }

  public ProcessService(
      Configurations serviceConfig,


The serviceConfig seems not be used.

zhoujinsong · 2026-03-04T12:55:46Z

amoro-ams/src/main/java/org/apache/amoro/server/AmoroServiceContainer.java

        new DefaultOptimizingService(serviceConfig, catalogManager, optimizerManager, tableService);

-    processService = new ProcessService(serviceConfig, tableService);
+    // Load process factories and build action coordinators from default table runtime factory.


// Load process factories and build action coordinators from default table runtime factory. TableProcessFactoryManager tableProcessFactoryManager = new TableProcessFactoryManager(); tableProcessFactoryManager.initialize(); List<ProcessFactory> processFactories = tableProcessFactoryManager.installedPlugins(); DefaultTableRuntimeFactory defaultRuntimeFactory = new DefaultTableRuntimeFactory(); defaultRuntimeFactory.initialize(processFactories); List<ActionCoordinator> actionCoordinators = defaultRuntimeFactory.supportedCoordinators(); ExecuteEngineManager executeEngineManager = new ExecuteEngineManager(); tableService = new DefaultTableService(serviceConfig, catalogManager, defaultRuntimeFactory); processService = new ProcessService(serviceConfig, tableService, actionCoordinators, executeEngineManager); optimizingService = new DefaultOptimizingService(serviceConfig, catalogManager, optimizerManager, tableService);

How about changing the code structure like this?

…y' into upstream/table-runtime-factory

xxubai · 2026-03-04T14:56:14Z

amoro-ams/src/main/java/org/apache/amoro/server/table/DefaultTableRuntimeFactory.java

+      }
+
+      // 2) Extra states from all process factories for this format (if any)
+      Map<Action, ProcessFactory> byAction = factoriesByFormat.get(format);


If DefaultTableRuntime.REQUIRED_STATES declares a state key with the same name as a ProcessFactory's required state but with a different type or default value, the factory's key silently overwrites the built-in one.

Suggestion: Add a type-compatibility check or throw on conflicting keys:

I will add a check to throw an exception when conflicts occur.

When a single ProcessFactory supports multiple actions for the same format, byAction.values() will contain that factory instance multiple times. Each invocation of factory.requiredStates() produces the same keys, which will hit the merged.containsKey check and throw an IllegalStateException

Fix: Deduplicate factories before iterating?

I have deduplicate factories.

xxubai · 2026-03-04T15:01:41Z

amoro-ams/src/main/java/org/apache/amoro/server/table/DefaultTableRuntimeFactory.java

-    return "default";
-  }
+  /** Coordinators derived from all installed process factories. */
+  private final List<ActionCoordinator> supportedCoordinators = Lists.newArrayList();


Callers can accidentally mutate the internal supportedCoordinators list. Return Collections.unmodifiableList(supportedCoordinators) instead.

xxubai · 2026-03-04T15:03:59Z

amoro-ams/src/main/java/org/apache/amoro/server/table/DefaultActionCoordinator.java

+  @Override
+  public long getNextExecutingTime(TableRuntime tableRuntime) {
+    // Fixed-rate scheduling based on configured trigger interval.
+    return strategy.getTriggerInterval().toMillis();


The method returns strategy.getTriggerInterval().toMillis() — a duration, not an absolute timestamp.
Do you mean System.currentTimeMillis() + interval?

The getNextExecutingTime method actually returns the duration. This is a legacy issue.

Maybe we should update the interface in a separate PR?

zhoujinsong

LGTM.

github-actions bot added the module:ams-server Ams server module label Feb 26, 2026

baiyangtx marked this pull request as ready for review February 26, 2026 13:32

baiyangtx changed the title ~~[Feature]: Load process factories via DefaultTableRuntimeFactory~~ [Improvement]: Load process factories via DefaultTableRuntimeFactory Feb 26, 2026

zhangyongxiang.alpha and others added 2 commits March 4, 2026 13:24

github-actions bot added the module:common label Mar 4, 2026

Merge branch 'master' into upstream/table-runtime-factory

7c7617c

zhoujinsong reviewed Mar 4, 2026

View reviewed changes

zhangyongxiang.alpha added 3 commits March 4, 2026 21:21

fix comment

0ca943c

Merge remote-tracking branch 'baiyangtx/upstream/table-runtime-factor…

4d81a08

…y' into upstream/table-runtime-factory

fix comment

425326a

xxubai reviewed Mar 4, 2026

View reviewed changes

zhangyongxiang.alpha added 2 commits March 5, 2026 12:10

fix comment

bc8175d

fix comment

9e43534

xxubai approved these changes Mar 5, 2026

View reviewed changes

zhoujinsong approved these changes Mar 5, 2026

View reviewed changes

baiyangtx merged commit 7ba51cd into apache:master Mar 5, 2026
6 checks passed

baiyangtx deleted the upstream/table-runtime-factory branch March 5, 2026 09:32

Conversation

baiyangtx commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Uh oh!

zhoujinsong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhoujinsong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

baiyangtx commented Feb 26, 2026 •

edited

Loading