[Refactor] Introduce Iceberg TableRuntimePlugin by majin1102 · Pull Request #4073 · apache/amoro

majin1102 · 2026-02-02T16:40:05Z

Why are the changes needed?

Refactoring work for format processing extensions

Brief change log

Abstract TableRuntimePlugin to extend lance table runtime processing
Refactor DefaultTableService to abstract a IcebergTablePlugin which implements TableRuntimeHandler(only aimed for Iceberg table)

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

codecov-commenter · 2026-02-07T13:06:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 22.39%. Comparing base (869860e) to head (4931482).
⚠️ Report is 14 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (869860e) and HEAD (4931482). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (869860e) HEAD (4931482)

core 1 0

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4073      +/-   ##
============================================
- Coverage     28.63%   22.39%   -6.24%     
+ Complexity     3942     2552    -1390     
============================================
  Files           654      458     -196     
  Lines         52293    42116   -10177     
  Branches       6621     5917     -704     
============================================
- Hits          14973     9433    -5540     
+ Misses        36230    31871    -4359     
+ Partials       1090      812     -278

Flag	Coverage Δ
core	`?`
trino	`22.39% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

czy006 · 2026-02-10T02:55:17Z

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

-      tableService.addHandlerChain(chain);
-    }
+  private List<TableRuntimePlugin> initTablePlugins() {
+    LOG.info("Setting up AMS table executors...");


Setting up AMS Table Plugins ...

xxubai · 2026-02-10T09:04:58Z

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

+  private List<TableRuntimePlugin> initTablePlugins() {
+    LOG.info("Setting up AMS table executors...");
+    InlineTableExecutors.getInstance().setup(tableService, serviceConfig);
+    IcebergTablePlugin icebergTablePlugin =


How to load different table runtime plugins?

I think we should still use SPI(PluginManager) to do this. I'm preparing to create a LanceTableRuntimePlugin to handle lance tables and we could see the path clearly.

But Iceberg table runtime plugin is automatically loaded.
How do you feel

xxubai · 2026-02-10T09:14:10Z

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

+
+  @Override
+  public boolean accept(ServerTableIdentifier tableIdentifier) {
+    return tableIdentifier.getFormat() == TableFormat.ICEBERG


IcebergTablePlugin will accept other formats? It should not be named as IcebergTablePlugin

I think TableFormatPlugin should not be implemented in the current PR. The current PR is for renaming and organizing, which I can provide later

IcebergTablePlugin will accept other formats? It should not be named as IcebergTablePlugin

I name this as IcebergTablePlugin because the current three are all based on Iceberg. I'm open to have another name. Do you have another idea.

How about IcebergLikeTablePlugin

xxubai · 2026-02-10T09:16:03Z

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

+
+  @Override
+  public void initialize(List<TableRuntime> tableRuntimes) {
+    if (headHandler != null) {


Already checked null in construct function

You are right

xxubai · 2026-02-10T09:20:41Z

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

-    }
+  private void initTableRuntimePlugins() {
+    List<TableRuntime> tableRuntimes = new ArrayList<>(tableRuntimeMap.values());
+    tableRuntimePlugins.forEach(plugin -> plugin.initialize(tableRuntimes));


Plugin is invoked in initTableRuntimes when create tableRuntime. Should we check the method workflow?

Plugin is initialized in this initTableRuntimePlugins, not creating table runtime? what do you mean by check the method workflow?

I'm not sure if it occur NPE when invoke plugin functions

xxubai · 2026-02-25T10:04:51Z

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

+
+  void initialize(List<TableRuntime> tableRuntimes);
+
+  void onTableCreated(@Nullable AmoroTable<?> amoroTable, TableRuntime tableRuntime);


The interface is named TableXXX. Should we simplify the method names, for example to onCreate?

xxubai · 2026-02-25T10:07:12Z

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

+
+import java.util.List;
+
+public interface TableRuntimePlugin {


1. TableRuntimePlugin violates Single Responsibility Principle
This interface mixes two distinct concerns:

Factory/decorator role: accept() + createTableRuntime() intercepts and decorates TableRuntime creation.

Lifecycle listener role: initialize() / onTableCreated() / onTableDropped() / dispose() observes table lifecycle events.

These should be separated. The factory/decorator logic belongs in the TableRuntimeFactory hierarchy (e.g. as a decorator pattern), while the lifecycle observation should be its own interface. Mixing them forces every plugin implementor to deal with both concerns even if they only care about one.

2. Semantic overlap between TableRuntimePlugin.accept() and TableRuntimeFactory.accept()
In DefaultTableService.createTableRuntime(), there is a two-phase accept chain: first TableRuntimeFactory.accept(identifier, properties) selects the factory, then TableRuntimePlugin.accept(identifier) selects the plugin to intercept creation. These two have overlapping semantics — both decide "does this component handle this table?" — but with inconsistent signatures (TableRuntimeFactory receives table properties, TableRuntimePlugin does not). This dual-accept flow is confusing and error-prone. Consider unifying the entry point so there is a single, clear decision path for table runtime creation.

majin1102 requested a review from baiyangtx February 2, 2026 16:40

github-actions bot added module:ams-server Ams server module module:common labels Feb 2, 2026

majin1102 force-pushed the tableruntimeplugin branch 3 times, most recently from 0f1a1a0 to 40120af Compare February 5, 2026 11:58

[Refactor] Introduce Iceberg TableRuntimePlugin

72eef46

majin1102 force-pushed the tableruntimeplugin branch from c3ccf93 to 72eef46 Compare February 7, 2026 09:32

fix ci

f3606a6

czy006 reviewed Feb 10, 2026

View reviewed changes

xxubai reviewed Feb 10, 2026

View reviewed changes

majin.nathan and others added 3 commits February 24, 2026 11:49

address comments

4931482

Merge branch 'master' into tableruntimeplugin

54da52d

fix ci

8d95bbb

xxubai reviewed Feb 25, 2026

View reviewed changes

rename IcebergBasedPlugin

7187903


		void initialize(List<TableRuntime> tableRuntimes);

		void onTableCreated(@Nullable AmoroTable<?> amoroTable, TableRuntime tableRuntime);

Conversation

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

Uh oh!

Why are the changes needed?

Brief change log

How was this patch tested?

Documentation

Uh oh!

codecov-commenter commented Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

majin1102 commented Feb 2, 2026 •

edited

Loading

codecov-commenter commented Feb 7, 2026 •

edited

Loading

majin1102 Feb 24, 2026 •

edited

Loading