Fixed the regression bug for CSAS/CTAS result type check. #419

hjafarpour · 2017-10-27T00:08:57Z

No description provided.

apurvam · 2017-10-27T06:39:16Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/KsqlResource.java

@@ -205,7 +205,7 @@ private KsqlEntity executeStatement(
            || statement instanceof DropStream
            || statement instanceof DropTable
    ) {
-      //   getStatementExecutionPlan(statement, statementText, streamsProperties);
+      getStatementExecutionPlan(statement, statementText, streamsProperties);


So the regression was that a statement was commented out? What was the impact exactly?

Yes, this line evaluates the query by generating the execution plan without executing it. This will detect if the CREATE TABLE AS SELECT and CREATE STREAM AS SELECT were correct or not. Without this check the wrong queries will be sent for execution.

@hjafarpour it doesn't use the return type, though? So are you saying that it is validating that they syntax is correct? If so, then maybe add a comment as such or have a method named validateStatementExecutionPlan - as it stands it looks like this method isn't doing anything

@dguy good point. I added comment to clarify the method call.

hjafarpour · 2017-10-27T17:44:16Z

retest this please

apurvam · 2017-10-27T21:14:25Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/KsqlResource.java

@@ -205,7 +205,8 @@ private KsqlEntity executeStatement(
            || statement instanceof DropStream
            || statement instanceof DropTable
    ) {
-      //   getStatementExecutionPlan(statement, statementText, streamsProperties);
+      //Sanity check for the statement before distributing it.
+      getStatementExecutionPlan(statement, statementText, streamsProperties);


I think this should be named validateExecutionPlan so its intended use is clear. The comment only helps people reading code in this context, but this method could be used in other contexts.

If this method is doing two things (ie. generating and validating the plan), then it would be better to split it up into two separate methods, each performing a particular function.

I agree w @apurvam - it should be renamed. Looking the code it doesn't make - sense. It looks like it is executing various ddlcommands inside getStatementExecutionPlan() (i.e. CreateTableAsSelect etc) - and also publishes them onto the command topic to allow all nodes to execute the same command again "distributeStatement()". Shouldnt we have a validate() - that doesn't do any execution and then call distribute()

Good point. I added a new method and updated the PR.

…he code.

apurvam · 2017-10-31T03:30:09Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/KsqlResource.java

+   */
+  private void validateStatement(Statement statement, String statementText,
+                                 Map<String, Object> streamsProperties) throws Exception {
+    getStatementExecutionPlan(statement, statementText, streamsProperties);


I am still confused about this. The getStatementExecutionPlan seems to just generate the full plan and returns an ExecutionPlan. If it fails, it throws an exception. What we are doing here is just running the whole process and then saying it's valid if no exception is thrown. I presume that the execution plan would have to be eventually generated again. This seems wasteful.

Why can't we retain the returned execution plan and use it directly?

Yes, we will generate the execution plan again but not at the same place but in the engine in every instance of the server. This validation is just executed in the server that has received the query from user. Also this validation does not alter the metastore while the other ones that we perform in the engines does.

apurvam · 2017-10-31T03:32:57Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

+    assertTrue("Incorrect response size.", result1.size() == 1);
+    assertThat(result1.get(0), instanceOf(ErrorMessageEntity.class));
+    ErrorMessageEntity errorMessageEntity1 = (ErrorMessageEntity) result1.get(0);
+    assertTrue(errorMessageEntity1.getErrorMessage().getMessage().equalsIgnoreCase("Invalid "


Does the exception thrown by validateExecutionPlan eventually boil down to this error message? If so, would it make more sense to test for validity at a more fine grained level, where we can check the typed exception directly?

Also, do we have tests for the positive code path, where there are no errors?

Yes, but the 'getStatementExecutionPlan' method is private and because of that I have the test in the caller.
The positive code path test needs to be added too as you pointed out but it will include much more that the fix for this regression bug and can be part of another PR that improves the test coverage for this KsqlResource.

…ype-Enforcement-Regression-Fix

dguy · 2017-10-31T10:26:27Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/KsqlResource.java

@@ -328,6 +342,8 @@ private ExecutionPlan getStatementExecutionPlan(Statement statement, String stat
    if (ddlCommandTask != null) {
      try {
        return new ExecutionPlan(ddlCommandTask.execute(statement, statementText, properties));
+      } catch (KsqlException ksqlException) {


Can we change this method so that it throw KsqlException rather than Exception - generally we shouldn't have methods with the signature blah() throws Exception unless we have no control over what is being thrown. Even then, we should probably catch and wrap such cases so we have the context of what has gone wrong etc

Agreed, updated it for this PR. When we did this we were still finalizing the KsqlException class. Updated it. Will have to look into other places to use KsqlException as much as possible in other PRs.

dguy · 2017-10-31T10:28:23Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

+    assertTrue("Incorrect response size.", result1.size() == 1);
+    assertThat(result1.get(0), instanceOf(ErrorMessageEntity.class));
+    ErrorMessageEntity errorMessageEntity1 = (ErrorMessageEntity) result1.get(0);
+    assertTrue(errorMessageEntity1.getErrorMessage().getMessage().equalsIgnoreCase("Invalid "


assertThat(errorMessage...toLowerCase(), equalTo("whatever is expected"))

Elsewhere to. assertTrue should only be used to check actual boolean conditions. For everything else we should be using assertThat

Updated the assertion checks.

hjafarpour · 2017-10-31T17:15:21Z

@dguy made your suggested changes.

hjafarpour · 2017-10-31T20:21:25Z

retest this please

dguy · 2017-11-01T11:01:00Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/computation/CommandStore.java

+        try {
+            commandProducer.send(new ProducerRecord<>(commandTopic, commandId, command)).get();
+        } catch (Exception e) {
+            throw new KsqlException("Could not write the statement into the command topic.", e);


Should we add the statementString in the exception method so we have a bit more context?

Sure, added the statement string to the message.

dguy · 2017-11-01T11:01:53Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/KsqlResource.java

            "Unable to execute statement '%s'",
            statementText
        ));
      } else {
-        throw new Exception("Unable to execute statement");
+        throw new KsqlException("Unable to execute statement");


Can we add the statementText into the exception so we know what weren't able to execute?

dguy · 2017-11-01T11:02:06Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/KsqlResource.java

@@ -239,6 +259,8 @@ private CommandStatusEntity distributeStatement(
      log.warn("Timeout to get commandStatus, waited {} milliseconds:, statementText:" + statementText,
                  distributedCommandResponseTimeout, exception);
      commandStatus = statementExecutor.getStatus(commandId).get();
+    } catch (Exception e) {
+      throw new KsqlException("Could not write the statement into the command topic.", e);


dguy · 2017-11-01T11:03:34Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

+    assertTrue("Incorrect response size.", result1.size() == 1);
+    assertThat(result1.get(0), instanceOf(ErrorMessageEntity.class));
+    ErrorMessageEntity errorMessageEntity1 = (ErrorMessageEntity) result1.get(0);
+    assertTrue(errorMessageEntity1.getErrorMessage().getMessage().equalsIgnoreCase("Invalid "


As i said above, we should use assertThat here too

dguy · 2017-11-01T11:04:30Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

+    KsqlResource testResource = TestKsqlResourceUtil.get();
+    String ksqlString1 = "CREATE STREAM s1 AS SELECT * FROM test_table;";
+
+    Response response1 = testResource.handleKsqlStatements(new KsqlRequest(ksqlString1, Collections


Here and elsewhere in this class, we can use Collections.emptyMap() instead and then we will not have the unchecked warnings

hjafarpour · 2017-11-01T20:18:10Z

@dguy added your suggestions.

dguy

Thanks @hjafarpour, LGTM

Fixed the regression bug for CSAS/CTAS result type check.

5305c4e

hjafarpour requested review from apurvam and dguy October 27, 2017 00:09

apurvam reviewed Oct 27, 2017

View reviewed changes

Added comments.

df94028

apurvam reviewed Oct 27, 2017

View reviewed changes

Added validate method for statement sanity check for more carity in t…

35e8660

…he code.

apurvam reviewed Oct 31, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/4.0.x' into KSQL-413-Resault-T…

8cd30d0

…ype-Enforcement-Regression-Fix

dguy reviewed Oct 31, 2017

View reviewed changes

Updated the exception to KsqlException.

65783e0

dguy reviewed Nov 1, 2017

View reviewed changes

Added statement text to the error message.

205a09f

dguy approved these changes Nov 2, 2017

View reviewed changes

hjafarpour merged commit 361dff0 into confluentinc:4.0.x Nov 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed the regression bug for CSAS/CTAS result type check. #419

Fixed the regression bug for CSAS/CTAS result type check. #419

hjafarpour commented Oct 27, 2017

apurvam Oct 27, 2017

hjafarpour Oct 27, 2017

dguy Oct 27, 2017

hjafarpour Oct 27, 2017

hjafarpour commented Oct 27, 2017

apurvam Oct 27, 2017

bluemonk3y Oct 30, 2017

hjafarpour Oct 30, 2017

apurvam Oct 31, 2017

hjafarpour Oct 31, 2017

apurvam Oct 31, 2017

apurvam Oct 31, 2017

hjafarpour Oct 31, 2017

dguy Oct 31, 2017

hjafarpour Oct 31, 2017 •

edited

Loading

dguy Oct 31, 2017

hjafarpour Oct 31, 2017

hjafarpour commented Oct 31, 2017

hjafarpour commented Oct 31, 2017

dguy Nov 1, 2017

hjafarpour Nov 1, 2017

dguy Nov 1, 2017

hjafarpour Nov 1, 2017

dguy Nov 1, 2017

hjafarpour Nov 1, 2017

dguy Nov 1, 2017

hjafarpour Nov 1, 2017

dguy Nov 1, 2017

hjafarpour Nov 1, 2017

hjafarpour commented Nov 1, 2017

dguy left a comment

Fixed the regression bug for CSAS/CTAS result type check. #419

Fixed the regression bug for CSAS/CTAS result type check. #419

Conversation

hjafarpour commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjafarpour commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjafarpour Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjafarpour commented Oct 31, 2017

hjafarpour commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjafarpour commented Nov 1, 2017

dguy left a comment

Choose a reason for hiding this comment

hjafarpour Oct 31, 2017 •

edited

Loading