[GitHub] [shardingsphere] tristaZero commented on a change in pull request #10121: Report execution process from one proxy for running SQLs by RQL

[GitBox <gi...@apache.org>]

tristaZero commented on a change in pull request #10121:
URL: https://github.com/apache/shardingsphere/pull/10121#discussion_r615515334



##########
File path: shardingsphere-governance/shardingsphere-governance-context/src/main/java/org/apache/shardingsphere/governance/context/process/GovernanceExecuteProcessReporter.java
##########
@@ -29,12 +35,14 @@
 public final class GovernanceExecuteProcessReporter implements ExecuteProcessReporter {
     
     @Override
-    public void report(final SQLStatementContext<?> context, final ExecutionGroupContext<? extends SQLExecutionUnit> executionGroupContext, final ExecuteProcessConstants constants) {
-        // TODO :Call API of configCenter
+    public void report(final SQLStatementContext<?> context, final ExecutionGroupContext<? extends SQLExecutionUnit> executionGroupContext) {
+        ExecuteProcessContext executeProcessContext = new ExecuteProcessContext(executionGroupContext);
+        ShardingSphereEventBus.getInstance().post(new ReportExecuteProcessSummaryEvent(new YamlExecuteProcessContext(executeProcessContext)));

Review comment:
       Why is a new class, i.e.,`YamlExecuteProcessContext` used here? `ExecutionGroupContext` is unsuitable?

##########
File path: shardingsphere-governance/shardingsphere-governance-context/src/main/java/org/apache/shardingsphere/governance/context/process/GovernanceExecuteProcessReporter.java
##########
@@ -29,12 +35,14 @@
 public final class GovernanceExecuteProcessReporter implements ExecuteProcessReporter {
     
     @Override
-    public void report(final SQLStatementContext<?> context, final ExecutionGroupContext<? extends SQLExecutionUnit> executionGroupContext, final ExecuteProcessConstants constants) {
-        // TODO :Call API of configCenter
+    public void report(final SQLStatementContext<?> context, final ExecutionGroupContext<? extends SQLExecutionUnit> executionGroupContext) {
+        ExecuteProcessContext executeProcessContext = new ExecuteProcessContext(executionGroupContext);
+        ShardingSphereEventBus.getInstance().post(new ReportExecuteProcessSummaryEvent(new YamlExecuteProcessContext(executeProcessContext)));

Review comment:
       It is suggested to consider renaming `ReportExecuteProcessUnitEvent` with `ExecuteProcessUnitReportEvent `. Similarly, `ReportExecuteProcessSummaryEvent` -> `ExecuteProcessSummaryReportEvent `.

##########
File path: shardingsphere-infra/shardingsphere-infra-executor/src/main/java/org/apache/shardingsphere/infra/executor/sql/process/spi/ExecuteProcessReporter.java
##########
@@ -31,9 +31,8 @@
      * Report the summary of this task.
      * @param context context
      * @param executionGroupContext execution group context
-     * @param constants constants
      */
-    void report(SQLStatementContext<?> context, ExecutionGroupContext<? extends SQLExecutionUnit> executionGroupContext, ExecuteProcessConstants constants);
+    void report(SQLStatementContext<?> context, ExecutionGroupContext<? extends SQLExecutionUnit> executionGroupContext);

Review comment:
       Is there no possibility that `ExecuteProcessConstants` is another value? How about report stoping, resuming status?




-- 
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:
users@infra.apache.org