[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.

["korlov42 (via GitHub)" <gi...@apache.org>]

korlov42 commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261149736


##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PlannerTimeoutTest.java:
##########
@@ -33,22 +34,51 @@
 import org.apache.calcite.rel.type.RelDataType;
 import org.apache.calcite.rel.type.RelDataTypeFactory;
 import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.ignite.internal.schema.NativeTypes;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders.ConfigurationParameter;
+import org.apache.ignite.internal.sql.engine.framework.TestCluster;
+import org.apache.ignite.internal.sql.engine.framework.TestNode;
 import org.apache.ignite.internal.sql.engine.prepare.IgnitePlanner;
 import org.apache.ignite.internal.sql.engine.prepare.PlanningContext;
 import org.apache.ignite.internal.sql.engine.rel.IgniteConvention;
 import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
 import org.apache.ignite.internal.sql.engine.schema.IgniteSchema;
+import org.apache.ignite.internal.sql.engine.util.SqlTestUtils;
 import org.jetbrains.annotations.NotNull;
 import org.junit.jupiter.api.Test;
 
 /**
  * Test planner timeout.
  */
 public class PlannerTimeoutTest extends AbstractPlannerTest {
-    private static final long PLANNER_TIMEOUT = 500;
+
+    /** Check correctness of planning timeout. */
+    @Test
+    public void testPlanningTimeout() {
+        TestCluster cluster = TestBuilders.cluster()

Review Comment:
   please don't do this. You don't need cluster to verify behaviour of some service



##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItCommonApiTest.java:
##########


Review Comment:
   why changes in this file are required? 



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgnitePlanner.java:
##########
@@ -560,6 +564,11 @@ public void checkCancel() {
                 long startTs = ctx.startTs();
 
                 if (FastTimestamps.coarseCurrentTimeMillis() - startTs > timeout) {
+                    LOG.info("Planning of a query aborted due to planner timeout threshold is reached [timeout={}, query={}]",

Review Comment:
   the same: conversation is resolved, but nothing has changed



##########
modules/core/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -203,6 +203,9 @@ public static class Sql {
 
         /** Execution cancelled. */
         public static final int EXECUTION_CANCELLED_ERR = SQL_ERR_GROUP.registerErrorCode(33);
+
+        /** Planning timeouted without any valid plan. */
+        public static final int PLANNING_TIMEOUTED_ERR = SQL_ERR_GROUP.registerErrorCode(34);

Review Comment:
   Does it make sense to reuse `EXECUTION_CANCELLED_ERR` with message "planning timeout" or something?



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -142,24 +156,39 @@ public void stop() throws Exception {
     /** {@inheritDoc} */
     @Override
     public CompletableFuture<QueryPlan> prepareAsync(ParsedResult parsedResult, BaseQueryContext ctx) {
-        try {
-            PlanningContext planningContext = PlanningContext.builder()
-                    .parentContext(ctx)
-                    .build();
+        CompletableFuture<QueryPlan> result;
+
+        PlanningContext planningContext = PlanningContext.builder()
+                .parentContext(ctx)
+                .query(ctx.query())
+                .plannerTimeout(plannerTimeout)
+                .build();
+
+        result = prepareAsync0(parsedResult, planningContext);
+
+        return result.exceptionally(ex -> {
+                    Throwable th = ExceptionUtils.unwrapCause(ex);
+                    if (planningContext.timeouted() && th instanceof RelOptPlanner.CannotPlanException) {
+                        LOG.info("Query plan is absent due to planing timeout is reached [query={}]", ctx.query());

Review Comment:
   this is the second line in the log about planning timeout.



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/BaseQueryContext.java:
##########
@@ -148,35 +148,35 @@ public List<MetadataHandler<?>> handlers(Class<? extends MetadataHandler<?>> hnd
 
     private final QueryCancel cancel;
 
+    private final String query;

Review Comment:
   @ygerzhedovich you've resolved the conversation, but neither replied nor changed the line in question, why?



-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org