zhoujinsong commented on PR #4116:
URL: https://github.com/apache/amoro/pull/4116#issuecomment-4030681314
整体方向很好，`AmoroProcess` 与 `TableProcessStore`
的关注点分离让架构更清晰，`LocalExecutionEngine` 插件化也是正确的演进方向。但有几个问题需要关注：
**1. `TableProcessHolder` 字段直接 `public` 暴露**
```java
public static class TableProcessHolder {
public final TableProcess process;
public final TableProcessStore store;
}
```
直接暴露 `public final` 字段违反封装原则，建议改用 getter 方法，或者 Java 16+ 可以用
`record`。目前调用方已经大量使用 `holder.store`、`holder.process` 这种直接访问模式，后续如果需要封装会比较麻烦。
**2. `ProcessFactory.availableExecuteEngines()` 缺少 default 实现**
```java
void availableExecuteEngines(Collection<ExecuteEngine> allAvailableEngines);
```
`ProcessFactory` 是接口，已有多个实现类。新增非 default 方法会导致所有现有实现类编译失败。如果该方法是为后续扩展预留，建议加
`default void availableExecuteEngines(...) {}`
空实现；另外目前代码里也没有看到实际调用这个方法的地方，是否是未完成的逻辑？
**3. `LocalExecutionEngine.getStatus()` 中 `cancelingInstances` 存在内存泄漏**
```java
Map<String, Future<?>> instances =
cancelingInstances.containsKey(processIdentifier) ? cancelingInstances :
activeInstances;
Future<?> future = instances.get(processIdentifier);
if (future.isDone()) {
instances.remove(processIdentifier); // 只对 activeInstances 有效
...
}
```
当 future 来自 `cancelingInstances` 且已完成时，`instances.remove()` 确实会从
`cancelingInstances` 中移除，但在 `future.isCancelled()` 分支里没有 remove：
```java
if (future.isCancelled()) {
instances.remove(processIdentifier); // 这里有
return ProcessStatus.CANCELED;
}
```
再仔细看，`isDone()` 为 true 时包含了 cancelled 状态，所以 `isCancelled()` 分支实际上不会在
`isDone()` 之后执行到……但逻辑顺序让人困惑，建议梳理清楚 `isDone()`/`isCancelled()` 的判断顺序，加注释说明意图。
**4. `TableProcessMeta.createProcessMeta()` 过早调用 `getSummary()`**
```java
tableProcessMeta.setSummary(process.getSummary());
```
`AmoroProcess` 的 JavaDoc 明确写着 `getSummary()` 是 *"called when process is
succeeded"*，但这里在 process 刚创建（PENDING 状态）时就调用了，此时返回的必然是空值或 null。建议初始化时传入空 Map，在
process 完成时再更新 summary。
**5. `EngineType` 16 字符长度限制来源不明**
```java
private static final int MAX_NAME_LENGTH = 16;
```
如果是数据库字段长度限制，请在注释里注明对应的 DDL 字段；如果没有实际约束，建议移除这个限制，或者至少加注释说明原因。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
