majin1102 commented on code in PR #4097:
URL: https://github.com/apache/amoro/pull/4097#discussion_r2835986820
##########
amoro-common/src/main/java/org/apache/amoro/Action.java:
##########
@@ -20,38 +20,33 @@
import org.apache.amoro.shade.guava32.com.google.common.base.Preconditions;
-import java.util.Arrays;
+import java.util.Locale;
+import java.util.Map;
import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
public final class Action {
private static final int MAX_NAME_LENGTH = 16;
-
- /** supported table formats of this action */
- private final TableFormat[] formats;
+ private static final Map<String, Action> registeredActions = new
ConcurrentHashMap<>();
private final String name;
- /**
- * the weight number of this action, the bigger the weight number, the
higher positions of
- * schedulers or front pages
- */
- private final int weight;
Review Comment:
The weight is not used so far.
But it was designed to be used what action should be prioritied if we have
multiple actions to schedule in one resouce group.
I was considering to use a priority here and add a default value here. WDYT
##########
amoro-common/src/main/java/org/apache/amoro/process/ProcessTriggerStrategy.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.process;
+
+import java.time.Duration;
+
+/** Process trigger strategy. */
+public final class ProcessTriggerStrategy {
Review Comment:
Why do we need this?
I originally thought we should always triggered process when we have new
snapshots, config changed and time based. And this should be unextensiable to me
##########
amoro-common/src/main/java/org/apache/amoro/table/TableRuntimeFactory.java:
##########
@@ -29,6 +31,10 @@
/** Table runtime factory. */
public interface TableRuntimeFactory extends ActivePlugin {
+ List<ActionCoordinator> supportedCoordinators();
Review Comment:
So this extension should be aware of the implementation of Coordinator?
I feel a little weried for Lance scenario. Can we provide an abstract
TableRuntime if the coordinator is really necessary here? (Seriouslly I think
this should be a legacy issue?)
##########
amoro-common/src/main/java/org/apache/amoro/process/ProcessFactory.java:
##########
@@ -44,5 +63,5 @@ public interface ProcessFactory {
* @param state state of the process
* @return target process which has not been submitted yet.
*/
- AmoroProcess recover(TableRuntime tableRuntime, TableProcessState state);
+ TableProcess recover(TableRuntime tableRuntime, TableProcessStore store);
Review Comment:
I think we should also use Optional here for the case something went wrong
with the recovering
##########
amoro-common/src/main/java/org/apache/amoro/TableRuntime.java:
##########
@@ -48,6 +48,10 @@ public interface TableRuntime {
*/
List<? extends TableProcessStore> getProcessStates(Action action);
+ void registerProcess(TableProcessStore processStore);
Review Comment:
I think we should always use table runtime to trigger a process instead of
register a process into table runtime.
If not, I don't get the meaning of putting factories into runtimes
--
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]
