[AMORO-3485] Introduce scheduler module and external resource container #3486

majin1102 · 2025-03-25T13:58:59Z

Why are the changes needed?

Close #3485.

Brief change log

Document: https://docs.google.com/document/d/1pEKneQXI_OIY7EU_XX0xwHeRFG0HHi37bb3SaNucI00/edit?tab=t.0

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)

github-actions · 2025-04-25T00:04:10Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@amoro.apache.org list. Thank you for your contributions.

xxubai · 2025-05-07T08:26:46Z

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

+  private final Lock processLock = new ReentrantLock();
+  private final ServerTableIdentifier tableIdentifier;
+  private final DefaultOptimizingState optimizingState;
+  private final Map<Action, ProcessFactory<? extends TableProcessState>> processFactoryMap =


Is it thread safe? I believe processFactoryMap is used by multiple threads.
We can remove the reentrant lock and use ConcurrentHashMap directly to have better performance.

It is thread safe by processLock.

We could improve this by ConcurrentHashMap, still the processLock is necessary to handle read-and-create atomicity

The code has committed, PTAL

xxubai · 2025-05-07T08:27:03Z

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

+  private final DefaultOptimizingState optimizingState;
+  private final Map<Action, ProcessFactory<? extends TableProcessState>> processFactoryMap =
+      new HashMap<>();
+  private final Map<Long, AmoroProcess<? extends TableProcessState>> processMap = new HashMap<>();


same as processFactoryMap

xxubai · 2025-05-07T08:52:13Z

amoro-common/src/main/java/org/apache/amoro/ManagedTableRuntime.java

+import org.apache.amoro.process.ProcessFactory;
+import org.apache.amoro.process.TableProcessState;
+
+public interface ManagedTableRuntime extends TableRuntime {


The functions in this interface are all about actions and processes.

I think we can split it into an interface called SupportsAmoroProcess. So, DefaultTableRuntime implements TableRuntime and SupportsAmoroProcess

Yep,

I think SupportsProcessPlugins better, means support pluggable factories

xxubai · 2025-05-07T08:56:16Z

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

+    return optimizingState;
+  }
+
+  public void recover(OptimizingProcess optimizingProcess) {


Expose addiational methods: recover, registerMetric, dispose.
How abuot invoke thoese methods via getOptimizingState().xxx?

Agree
This could be introduced by another PR. I did't involve these to keep PR relatively small

xxubai · 2025-05-07T08:58:16Z

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

@@ -71,7 +71,7 @@ public class DefaultTableService extends PersistentBase implements TableService
  public static final Logger LOG = LoggerFactory.getLogger(DefaultTableService.class);
  private final long externalCatalogRefreshingInterval;

-  private final Map<Long, TableRuntime> tableRuntimeMap = new ConcurrentHashMap<>();
+  private final Map<Long, DefaultTableRuntime> tableRuntimeMap = new ConcurrentHashMap<>();


why use DefaultTableRuntime instead of TableRuntime. I don't see any methods which only belong to DefaultTableRuntime

like getOptimizingState()?
May be we can use ManagedTableRuntime to take place of DefaultTableRuntime, which would cause quite a lot of work. The previos POJO here is old TableRuntime, which is replaced by DefaultTableRuntime.getOptimizingState(), which has not been asbtracted to TableRuntime

xxubai · 2025-05-07T09:09:15Z

amoro-common/src/main/java/org/apache/amoro/resource/InternalResourceContainer.java

+ * AMS. Resources are decoupled from processes. For ExternalResourceContainer, resources are managed
+ * outside, and each single resource belonged to a process.
+ */
+public interface InternalResourceContainer extends ResourceContainer {


Do you mean the life cycle is managed by AMS?
Maybe remove the description about "external resource container"

I've improved the description. PTAL

xxubai · 2025-05-07T09:15:44Z

amoro-common/src/main/java/org/apache/amoro/resource/ExternalResourceContainer.java

+ * ExternalResourceContainer is the key interface for the AMS framework to interact with the
+ * external resource. Typically, it is used to submit and release resource on YARN/K8S.
+ */
+public interface ExternalResourceContainer extends ResourceContainer {


Do you mean the life cycle of ExternalResourceContainer is not managed by AMS?

Typically, it is used to submit and release resource on YARN/K8S.

I don't quite understand here. If AMS itself runs on K8S, then InternalResourceContainer will also run on K8S?What is the difference between InternalResourceContainer and ExternalResourceContainer?

For InternalContainer executors，they recieve tasks scheduled by AMS, and released by AMS when necessary. For ExternalContainer executors, they recieved tasks by some JobManager or application master. The true diff is that ResourceManager(response for task scheduling and managing lifetime of executors) is inside AMS or outside

I think we can make that clear in the comments.

xxubai · 2025-05-07T09:16:27Z

amoro-common/src/main/java/org/apache/amoro/IcebergActions.java

+  private static final TableFormat[] DEFAULT_FORMATS =
+      new TableFormat[] {TableFormat.ICEBERG, TableFormat.MIXED_ICEBERG, TableFormat.MIXED_HIVE};
+
+  public static final Action SYSTEM = new Action(DEFAULT_FORMATS, 0, "system");


What is SYSTEM meanning

It's the legacy process type like expiring process records, or just use it to take place all types of actions which don't have an action name

should add action name for constructor like above

xxubai · 2025-05-07T09:20:50Z

amoro-common/src/main/java/org/apache/amoro/process/ManagedProcess.java

+  void complete();
+
+  /** Mark this process as failed, trigger callbacks */
+  void complete(String failedReason);


Suggested change

void complete(String failedReason);

void failed(String failedReason);

This is quite different.
The complete(String failReason) is called by task completion phase. and failing could happen at any phase(like submitting), so I think complete(String failReason) is more reasonable here which means only completion callback would use this method

how about define one method with a flag indicate the process is success or not

What's the point or difference?

Because these two methods seem to perform different execution logic, but the two method names are the same, so I recommend differentiating the names

I think 'flag' no better，'failed' has the syntax of lifetime issue
Could you give a better idea of " complete with an error" method naming?

xxubai · 2025-05-07T09:24:00Z

amoro-common/src/main/java/org/apache/amoro/process/TableProcessState.java

@@ -30,6 +30,7 @@ public class TableProcessState implements ProcessState {
  @StateField private volatile long id;
  private final Action action;


I see the action field is materialized in the DB?

Yes
What's the problem?

Need to add @StateField for action field?

The field shoud never change. so action and identifer are both unnecessary for @StateField

majin1102 · 2025-05-12T08:18:55Z

@xxubai Thanks for reviewing. I've done some improvement. PTAL

xxubai · 2025-05-12T14:03:44Z

amoro-common/src/main/java/org/apache/amoro/SupportsProcessPlugins.java

+import org.apache.amoro.process.ProcessFactory;
+import org.apache.amoro.process.TableProcessState;
+
+public interface SupportsProcessPlugins extends TableRuntime {


I think the interface can be separate, rather than inheriting from TableRuntime

yep, it's better

xxubai · 2025-05-12T14:11:05Z

amoro-common/src/main/java/org/apache/amoro/process/ManagedProcess.java

+  void complete();
+
+  /** Mark this process as failed, trigger callbacks */
+  void complete(String failedReason);


Because these two methods seem to perform different execution logic, but the two method names are the same, so I recommend differentiating the names

amoro-ams/src/main/java/org/apache/amoro/server/persistence/mapper/ProcessStateMapper.java

amoro-ams/src/main/java/org/apache/amoro/server/scheduler/TimelyTableScheduler.java

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

sqd · 2025-05-13T17:33:02Z

amoro-ams/src/main/java/org/apache/amoro/server/scheduler/TimelyExternalScheduler.java

+  @Override
+  protected long getNextExecutingTime(DefaultTableRuntime tableRuntime) {
+    return 0;
+  }


This essentially schedules the task in a while(true) loop. Intentional?

This method should be implemented by child classes. it should be deleted.
PTAL

xxubai

Overall, it looks pretty good, but there are still some minor issues, such as upgrading SQL script and adding more test cases.

However, these can be addressed in other PRs.

Can you double check this PR @zhoujinsong

Aireed

I left some comments, PTAL

amoro-ams/src/main/java/org/apache/amoro/server/persistence/PersistentBase.java

amoro-ams/src/main/java/org/apache/amoro/server/persistence/mapper/ProcessStateMapper.java

Aireed · 2025-05-16T06:06:38Z

amoro-ams/src/main/java/org/apache/amoro/server/persistence/mapper/ProcessStateMapper.java

+      "UPDATE table_process_state "
+          + "SET status = #{status}, end_time = #{endTime}, fail_reason = #{failedReason} "
+          + "WHERE process_id = #{id} and retry_num = #{retryNumber}")
+  void updateProcessFailed(TableProcessState state);


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

amoro-ams/src/main/resources/mysql/ams-mysql-init.sql

amoro-common/src/main/java/org/apache/amoro/Action.java

Aireed · 2025-05-16T06:41:15Z

amoro-common/src/main/java/org/apache/amoro/IcebergActions.java

+  private static final TableFormat[] DEFAULT_FORMATS =
+      new TableFormat[] {TableFormat.ICEBERG, TableFormat.MIXED_ICEBERG, TableFormat.MIXED_HIVE};
+
+  public static final Action SYSTEM = new Action(DEFAULT_FORMATS, 0, "system");


should add action name for constructor like above

amoro-common/src/main/java/org/apache/amoro/hive/AuthenticatedHiveClientPool.java

Aireed · 2025-05-16T06:45:24Z

amoro-common/src/main/java/org/apache/amoro/process/ProcessStatus.java

@@ -26,6 +26,7 @@ public enum ProcessStatus {
  RUNNING,
  SUCCESS,
  CLOSED,


CLOSED should be removed for I don't see CLOSED being used again.

Check OptimizingQueue.java

Check OptimizingQueue.java

i still can't found CLOSED using in OptimizingQueue.java. Please share me the code

majin1102 · 2025-05-16T09:36:42Z

Overall, it looks pretty good, but there are still some minor issues, such as upgrading SQL script and adding more test cases.

However, these can be addressed in other PRs.

Can you double check this PR @zhoujinsong

There's much work to do, like how to implement ExternalScheduler, where to put it, how to initialize it.
For some reasons I won't have much time to follow, this PR is mainly to complete the api and point out the path for constructing processes and the way to impelement schedulers(extensions need to manage lifetimes of external processes including tracking status).

I look forward to seeing more documents and extensions based on this

…ptimizingState.java Co-authored-by: Wang Tao <watters.tao@gmail.com>

…HiveClientPool.java Co-authored-by: Wang Tao <watters.tao@gmail.com>

codecov-commenter · 2025-05-16T10:46:32Z

Codecov Report

Attention: Patch coverage is 42.41645% with 224 lines in your changes missing coverage. Please review.

Project coverage is 28.25%. Comparing base (d7d6534) to head (a0bb902).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
...ro/server/scheduler/PeriodicExternalScheduler.java	0.00%	79 Missing ⚠️
...apache/amoro/server/table/DefaultTableRuntime.java	53.70%	25 Missing ⚠️
...va/org/apache/amoro/process/TableProcessState.java	0.00%	19 Missing ⚠️
...pache/amoro/server/optimizing/OptimizingQueue.java	67.30%	5 Missing and 12 partials ⚠️
...amoro/server/scheduler/PeriodicTableScheduler.java	26.66%	11 Missing ⚠️
...che/amoro/server/table/DefaultOptimizingState.java	56.00%	11 Missing ⚠️
.../scheduler/inline/TableRuntimeRefreshExecutor.java	41.17%	7 Missing and 3 partials ⚠️
...-common/src/main/java/org/apache/amoro/Action.java	0.00%	9 Missing ⚠️
...amoro/server/optimizing/sorter/BalancedSorter.java	0.00%	8 Missing ⚠️
...src/main/java/org/apache/amoro/IcebergActions.java	0.00%	7 Missing ⚠️
... and 14 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3486      +/-   ##
============================================
+ Coverage     21.76%   28.25%   +6.48%     
- Complexity     2391     3702    +1311     
============================================
  Files           431      614     +183     
  Lines         40498    49765    +9267     
  Branches       5744     6433     +689     
============================================
+ Hits           8816    14063    +5247     
- Misses        30935    34693    +3758     
- Partials        747     1009     +262

Flag	Coverage Δ
core	`28.25% <42.41%> (?)`
trino	`?`

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.

…rsistentBase.java Co-authored-by: Wang Tao <watters.tao@gmail.com>

majin1102 · 2025-05-27T09:43:42Z

I left some comments, PTAL

Thanks for reviewing, I have updated the PR, PTAL

Aireed

LGTM

github-actions bot added module:ams-server Ams server module type:build module:common labels Mar 25, 2025

github-actions bot added the stale label Apr 25, 2025

majin1102 force-pushed the external-api branch from d3b295b to 71adc81 Compare April 27, 2025 12:25

github-actions bot removed the type:build label Apr 27, 2025

majin1102 changed the title ~~[AMORO-3485][WIP] Introduce scheduler module and external resource container~~ [AMORO-3485] Introduce scheduler module and external resource container Apr 27, 2025

majin1102 force-pushed the external-api branch from 71adc81 to 71c1f7b Compare April 27, 2025 12:37

github-actions bot removed the stale label Apr 28, 2025

xxubai reviewed May 7, 2025

View reviewed changes

majin1102 force-pushed the external-api branch from 71c1f7b to 8572860 Compare May 12, 2025 08:17

[AMORO-3485] Introduce scheduler module and external resource container

60e31de

majin1102 force-pushed the external-api branch from 8572860 to 60e31de Compare May 12, 2025 09:53

majin.nathan added 2 commits May 12, 2025 21:44

Improve the TableRuntime and ProcessFactory logics

5191c21

Improve comment of InternalResourceContainer

4ef0081

xxubai reviewed May 12, 2025

View reviewed changes

sqd reviewed May 13, 2025

View reviewed changes

amoro-ams/src/main/java/org/apache/amoro/server/table/DefaultTableRuntime.java Outdated Show resolved Hide resolved

sqd reviewed May 13, 2025

View reviewed changes

Improve classes structure

5df3713

majin1102 force-pushed the external-api branch from ecd23ac to 5df3713 Compare May 15, 2025 07:07

majin.nathan and others added 2 commits May 15, 2025 15:18

Improve classes naming and comment

748d045

Merge branch 'master' into external-api

3f01ed0

xxubai approved these changes May 15, 2025

View reviewed changes

Aireed requested changes May 16, 2025

View reviewed changes

majin1102 and others added 3 commits May 16, 2025 17:47

Update amoro-ams/src/main/java/org/apache/amoro/server/table/DefaultO…

78c81c1

…ptimizingState.java Co-authored-by: Wang Tao <watters.tao@gmail.com>

Update amoro-common/src/main/java/org/apache/amoro/hive/Authenticated…

2ee537d

…HiveClientPool.java Co-authored-by: Wang Tao <watters.tao@gmail.com>

Improvement by reviewing

21ce674

spotless apply

a0bb902

xxubai and others added 3 commits May 22, 2025 22:40

Merge branch 'master' into external-api

a4e4100

Merge branch 'master' into external-api

de295be

Update amoro-ams/src/main/java/org/apache/amoro/server/persistence/Pe…

4962118

…rsistentBase.java Co-authored-by: Wang Tao <watters.tao@gmail.com>

Aireed approved these changes May 27, 2025

View reviewed changes

xxubai merged commit 064f400 into apache:master May 28, 2025
4 checks passed

	void complete(String failedReason);
	void failed(String failedReason);

		@@ -30,6 +30,7 @@ public class TableProcessState implements ProcessState {
		@StateField private volatile long id;
		private final Action action;

[AMORO-3485] Introduce scheduler module and external resource container #3486

[AMORO-3485] Introduce scheduler module and external resource container #3486

Uh oh!

Conversation

majin1102 commented Mar 25, 2025 • 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!

github-actions bot commented Apr 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majin1102 May 12, 2025 • 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!

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!

majin1102 May 12, 2025 • 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!

majin1102 May 15, 2025 • 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!

majin1102 May 12, 2025 • 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

majin1102 commented Mar 25, 2025 •

edited

Loading

majin1102 May 12, 2025 •

edited

Loading

majin1102 May 12, 2025 •

edited

Loading

majin1102 May 15, 2025 •

edited

Loading

majin1102 May 12, 2025 •

edited

Loading

majin1102 commented May 12, 2025 •

edited

Loading

majin1102 May 15, 2025 •

edited

Loading