zhoujinsong commented on code in PR #4100:
URL: https://github.com/apache/amoro/pull/4100#discussion_r2883487421
##########
amoro-common/src/main/java/org/apache/amoro/table/TableRuntimeFactory.java:
##########
@@ -29,7 +28,7 @@
import java.util.Optional;
/** Table runtime factory. */
-public interface TableRuntimeFactory extends ActivePlugin {
+public interface TableRuntimeFactory {
Review Comment:
Do we still need other table runtime factory implementation now, or is the
default the only one?
##########
amoro-ams/src/main/java/org/apache/amoro/server/process/ProcessService.java:
##########
@@ -64,8 +64,8 @@ public class ProcessService extends PersistentBase {
new ConcurrentHashMap<>();
private final Map<EngineType, ExecuteEngine> executeEngines = new
ConcurrentHashMap<>();
- private final ActionCoordinatorManager actionCoordinatorManager;
Review Comment:
It seems the `ActionCoordinatorManager` class can be dropped now.
##########
amoro-ams/src/main/java/org/apache/amoro/server/AmoroServiceContainer.java:
##########
@@ -229,16 +233,26 @@ public void transitionToFollower() {
}
public void startOptimizingService() throws Exception {
- TableRuntimeFactoryManager tableRuntimeFactoryManager = new
TableRuntimeFactoryManager();
- tableRuntimeFactoryManager.initialize();
+ DefaultTableRuntimeFactory defaultRuntimeFactory = new
DefaultTableRuntimeFactory();
- tableService =
- new DefaultTableService(serviceConfig, catalogManager,
tableRuntimeFactoryManager);
+ tableService = new DefaultTableService(serviceConfig, catalogManager,
defaultRuntimeFactory);
optimizingService =
new DefaultOptimizingService(serviceConfig, catalogManager,
optimizerManager, tableService);
- processService = new ProcessService(serviceConfig, tableService);
+ // Load process factories and build action coordinators from default table
runtime factory.
Review Comment:
```
// 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?
##########
amoro-ams/src/main/java/org/apache/amoro/server/process/ProcessService.java:
##########
@@ -74,16 +74,16 @@ public class ProcessService extends PersistentBase {
new ConcurrentHashMap<>();
public ProcessService(Configurations serviceConfig, TableService
tableService) {
- this(serviceConfig, tableService, new ActionCoordinatorManager(), new
ExecuteEngineManager());
+ this(serviceConfig, tableService, Collections.emptyList(), new
ExecuteEngineManager());
}
public ProcessService(
Configurations serviceConfig,
Review Comment:
The `serviceConfig` seems not be used.
--
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]
