tristaZero commented on a change in pull request #10351:
URL: https://github.com/apache/shardingsphere/pull/10351#discussion_r633047599
##########
File path:
shardingsphere-infra/shardingsphere-infra-executor/src/main/java/org/apache/shardingsphere/infra/executor/sql/process/ExecuteProcessEngine.java
##########
@@ -55,6 +55,13 @@ public static void initialize(final SQLStatementContext<?>
context, final Execut
}
}
+ /**
+ * Clean up execution ID.
+ */
+ public static void cleanupExecutionID() {
+
ExecutorDataMap.getValue().remove(ExecuteProcessConstants.EXECUTE_ID.name());
Review comment:
Could we move this statement to `void finish(final String executionID)
` and remove this function?
##########
File path:
shardingsphere-infra/shardingsphere-infra-executor/src/main/java/org/apache/shardingsphere/infra/executor/sql/process/ExecuteProcessStrategyEvaluator.java
##########
@@ -38,6 +39,6 @@
*/
public static boolean evaluate(final SQLStatementContext<?> context, final
ExecutionGroupContext<? extends SQLExecutionUnit> executionGroupContext) {
// TODO : Add more conditions to evaluate whether to submit this
process task or not
- return false;
+ return context.getSqlStatement() instanceof DDLStatement;
Review comment:
I suggest we'd better retain false before we complete finish and test
this feature.
##########
File path:
shardingsphere-infra/shardingsphere-infra-executor/src/main/java/org/apache/shardingsphere/infra/executor/sql/execute/engine/driver/jdbc/JDBCExecutorCallback.java
##########
@@ -66,6 +66,9 @@
result.add(executeResult);
}
}
+ if (dataMap.containsKey(ExecuteProcessConstants.EXECUTE_ID.name())) {
Review comment:
Why do not we put this into `DriverJDBCExecutor` so that it will
correspond to `initialize()`?
##########
File path:
shardingsphere-infra/shardingsphere-infra-executor/src/main/java/org/apache/shardingsphere/infra/executor/sql/process/spi/ExecuteProcessReporter.java
##########
@@ -42,4 +42,11 @@
* @param constants constants
*/
void report(String executionID, SQLExecutionUnit executionUnit,
ExecuteProcessConstants constants);
+
+ /**
+ * Finish this task.
+ * @param executionID execution ID
+ * @param constants constants
+ */
+ void finish(String executionID, ExecuteProcessConstants constants);
Review comment:
Do you think it is better to rename it as `report()`?
##########
File path:
shardingsphere-infra/shardingsphere-infra-executor/src/main/java/org/apache/shardingsphere/infra/executor/sql/execute/engine/driver/jdbc/JDBCExecutorCallback.java
##########
@@ -66,6 +66,9 @@
result.add(executeResult);
}
}
+ if (dataMap.containsKey(ExecuteProcessConstants.EXECUTE_ID.name())) {
Review comment:
So we can not move these coding into `ExecuteProcessEngine` as
`finish()` like other functions in this class?
Plus, we have called `ExecuteProcessEngine.initialize` in all the places. I
mean `JDBCExecutorCallback` is for SQL process and report each child status,
on the other hand, `DriverJDBCExecutor` is the main entrance for `initialize`
and `finish`.
##########
File path:
shardingsphere-infra/shardingsphere-infra-executor/src/main/java/org/apache/shardingsphere/infra/executor/sql/process/ExecuteProcessEngine.java
##########
@@ -55,6 +55,13 @@ public static void initialize(final SQLStatementContext<?>
context, final Execut
}
}
+ /**
+ * Clean up execution ID.
+ */
+ public static void cleanupExecutionID() {
+
ExecutorDataMap.getValue().remove(ExecuteProcessConstants.EXECUTE_ID.name());
Review comment:
Maybe `initialize()`, `finish()` and `clean()` could form as,
```java
try {
ExecuteProcessEngine.initialize(sqlStatementContext,
executionGroupContext);
List<Integer> results =
jdbcLockEngine.execute(executionGroupContext, sqlStatementContext, routeUnits,
callback); // Child report
ExecuteProcessEngine.finish(); // Final report
return result;
} catch (final Exception ex) {
ExecuteProcessEngine.clean();
}
```
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
