fix: allow insert into table using HTTP/2 endpoint and Java API #8114

lihaosky · 2021-09-08T23:30:09Z

Description

Inserting into tables using CLI is permitted but using Java API is not. Allow it in Java API to make the behavior consistent.

Fixes #7948

Changes

Remove checks for table insertion in ksqldb-rest-app/src/main/java/io/confluent/ksql/api/impl/InsertsStreamEndpoint.java.
Remove two unit tests in ksqldb-api-client/src/test/java/io/confluent/ksql/api/client/ClientTest.java as they use testEndpoint and doesn't actually test anything in production endpoint.
Add 4 test in integration test. All other changes in Integration test comes from Intellij auto formatting
- Insert and Pull from table -> Exception
- Insert and Push query from table -> Success
- Insert and Pull from CTAS table -> Nothing returns
- Insert to topic and Pull from CTAS table -> Success

mjsax · 2021-09-10T02:05:38Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java


    // Then: should receive new row
    final String query = "SELECT * FROM " + EMPTY_TEST_STREAM + " EMIT CHANGES LIMIT 1;";
+    final StreamedQueryResult streamedQueryResult = client.streamQuery(PULL_QUERY_ON_TABLE).get();


Seems this is unused?

Good catch 👍

mjsax · 2021-09-10T02:08:48Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

@@ -657,22 +679,111 @@ public void shouldInsertInto() throws Exception {
  }

  @Test
-  public void shouldHandleErrorResponseFromInsertInto() {
+  public void shouldPullQueryTableAfterInsert() throws Exception {


A regular table should not be pull-queryable? Is the test name incorrect?

Overall, I am wondering what you try to do: insert into the table and verify that the data was written? For this case you could use a push query in the test.

Yeah, test name should be shouldNot...

I'm trying to test you can insert into table created using with but pull query it will throw exception

My question is really: this PR is about allowing INSERT INTO for tables -- however, INSERT INTO and pull-queries have nothing to do with each other. So what do we gain by issuing a pull query (that we know is not supported for this case anyway)? -- Overall, it seems that the second part of this test is unnecessary and the first part is covered by the next test that uses a push query to verify if the insert was successful. Thus, should we remove this test?

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

mjsax · 2021-09-10T02:12:32Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

+  }
+
+  @Test
+  public void shouldPullQueryCTASTableAfterInsert() throws Exception {


Not sure if this test buys us much?

Trying to make sure direct insert to CTAS table will have no effect

What do you mean by "will not affected" exaclty?

I mean you can insert to the table successfully but you won't see the result in pull request

I mean you can insert to the table successfully but you won't see the result in pull request

Well, while this is true, to me it's rather a design flaw / bug than a feature. Thus, I am not sure if it makes sense to test this case?

+1 to Matthias: I don't think this test makes sense in this file. This test file is meant to test client-specific behavior. If the purpose of this test is to verify that INSERT VALUES does not update pull query state stores, then the test does not belong in this file. (In general, integration tests are (relatively) expensive so I'd err against adding redundant/duplicate tests.)

Sure. I'll remove this

mjsax · 2021-09-10T02:16:30Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

+  }
+
+  @Test
+  public void shouldPullQueryCTASTableAfterInsertStream() throws Exception {


insert into stream did always work, so it seems the test is actually not related to this PR?

Want to test inserting to stream which backs CTAS table and it will be inserted to table. I'm trying to cover all possible ways to insert to a table.

If this closes a test gap, great. I was just wondering if this case was already covered or not.

Yeah. Previously in integration test we have a CTAS table AGG_TABLE. This test case's table is a new table created using WITH and we are inserting to the topic

Ditto to my comment above -- I'm not convinced this test belongs in this file. I definitely think we should have a test that verifies that INSERT VALUES is allowed for tables, but I don't think we need to test auxiliary behaviors.

I can remove this as well

mjsax · 2021-09-10T02:17:12Z

ksqldb-engine/src/test/java/io/confluent/ksql/integration/IntegrationTestHarness.java

@@ -133,7 +133,7 @@ public boolean topicExists(final String topicName) {
  /**
   * Ensure topics with the given {@code topicNames} exist.
   *
-   * <p>Topics will be creates, if they do not already exist, with a single partition and replica.
+   * <p>Topics will be created, if they do not already exist, with a single partition and replica.


Nice catch!

mjsax · 2021-09-10T03:57:44Z

Seems there is some spotbug issue:

[ERROR] Medium: Dead store to streamedQueryResult in io.confluent.ksql.api.client.integration.ClientIntegrationTest.shouldInsertInto() [io.confluent.ksql.api.client.integration.ClientIntegrationTest] At ClientIntegrationTest.java:[line 660] DLS_DEAD_LOCAL_STORE

Might be fixed already by removing the unused variable.

lihaosky

Seems there is some spotbug issue:

[ERROR] Medium: Dead store to streamedQueryResult in io.confluent.ksql.api.client.integration.ClientIntegrationTest.shouldInsertInto() [io.confluent.ksql.api.client.integration.ClientIntegrationTest] At ClientIntegrationTest.java:[line 660] DLS_DEAD_LOCAL_STORE

Might be fixed already by removing the unused variable.

Yeah. It's checkstyle failure and I fixed it

lihaosky · 2021-09-10T16:49:26Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

+  }
+
+  @Test
+  public void shouldPullQueryCTASTableAfterInsertStream() throws Exception {


Yeah. Previously in integration test we have a CTAS table AGG_TABLE. This test case's table is a new table created using WITH and we are inserting to the topic

lihaosky · 2021-09-10T16:57:16Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

@@ -657,22 +678,111 @@ public void shouldInsertInto() throws Exception {
  }

  @Test
-  public void shouldHandleErrorResponseFromInsertInto() {
+  public void shouldHandlePullQueryTableExceptionAfterInsert() throws Exception {


I think you had a comment here in older commit but it's gone after I updated this file...

My question is really: this PR is about allowing INSERT INTO for tables -- however, INSERT INTO and pull-queries have nothing to do with each other. So what do we gain by issuing a pull query (that we know is not supported for this case anyway)? -- Overall, it seems that the second part of this test is unnecessary and the first part is covered by the next test that uses a push query to verify if the insert was successful. Thus, should we remove this test?

I guess this could be two test case:

Insert into table

Table created with WITH is not queryable.

I don't see 2 is covered here so added it. Maybe I can split it to 2 test cases to make it clearer?

mjsax

Overall LGTM. Call for second review @vcrfxia to sanity check, this part of the code is also new to me. -- In particular if we should/need to test "insert into" a table created via CTAS.

mjsax · 2021-09-14T23:51:41Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

+  }
+
+  @Test
+  public void shouldPullQueryCTASTableAfterInsert() throws Exception {


I mean you can insert to the table successfully but you won't see the result in pull request

Well, while this is true, to me it's rather a design flaw / bug than a feature. Thus, I am not sure if it makes sense to test this case?

vcrfxia

Thanks for the fix @lihaosky ! Some comments about test coverage inline.

I also updated the PR title to be more informative to users for when it's eventually included in the changelog for a release.

vcrfxia · 2021-09-15T06:26:57Z

ksqldb-api-client/src/test/java/io/confluent/ksql/api/client/ClientTest.java

@@ -591,24 +591,6 @@ public void shouldInsertInto() throws Exception {
    assertThat(testEndpoints.getLastTarget(), is("test-stream"));
  }

-  @Test
-  public void shouldHandleErrorResponseFromInsertInto() {


Why are these tests being removed? They are validating that an informative 400 response error message is returned to the user. We can update the exception message to be more realistic (in light of the changes in this PR) but I think we should keep the test coverage.

But an "insert into" should be supported now? What kind of error do you have in mind that we should test?

The purpose of this test is to check that any error from the insert values endpoint is properly propagated to the user. If you look at the code for the endpoint, you'll see that there are plenty of other errors that could be triggered (besides attempting to insert into a table), e.g., trying to insert into a non-existent source.

Deleting this test opens us up to potential regressions where the client does not properly propagate error messages from the server response to the user. We should not delete this coverage.

Hi @vcrfxia, do you mean we should also test handling error like these:
https://github.com/confluentinc/ksql/blob/master/ksqldb-rest-app/src/main/java/io/confluent/ksql/api/impl/InsertsStreamEndpoint.java#L61-L80

I agree we should cover those but that seems belong to another PR since these PR is about allowing insert to table. I can create another issue about above missing tests and fix those in a separate PR. What do you think?

I'm saying we should have one test to validate that any error is properly propagated from the server to the user (via the client). We do not need to test every possible server error. The proper place for testing each possible server error would be in a unit/integration test for the server endpoint, not the client. The client tests should just contain a single test to validate that any error is properly propagated to the user.

I think we should simply update the test highlighted in this comment (in this PR) to test a different error, rather than removing the test in this PR and opening a follow-up PR to add it back.

vcrfxia · 2021-09-15T06:27:42Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

@@ -255,7 +259,12 @@ public static void setUpClass() throws Exception {
        AGG_SCHEMA
    );

-    final String testDir = Paths.get(TestUtils.tempDirectory().getAbsolutePath(), "client_integ_test").toString();
+    makeKsqlRequest(


nit: can we move this up a few lines to be alongside the other makeKsqlRequest()?

vcrfxia · 2021-09-15T06:31:32Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

@@ -657,22 +678,103 @@ public void shouldInsertInto() throws Exception {
  }

  @Test
-  public void shouldHandleErrorResponseFromInsertInto() {
+  public void shouldHandlePullQueryTableException() throws Exception {


Why is this test being added? Testing the specific error (query against a table this is not query-able) doesn't make sense in this class since it's not specific to the client. Regarding client behavior for this error, this test file already contains shouldHandleErrorResponseFromExecuteQuery() and shouldHandleErrorResponseFromStreamQuery().

I'm trying to cover more error cases. This is client behavior triggering exception by query non-queryable table. shouldHandleErrorResponseFromExecuteQuery and shouldHandleErrorResponseFromStreamQuery seem cover other behavior like query non-exist table. I feel query non-queryable table is also one of the cases. If you feel this is not needed, I can remove this

As mentioned above, whether the table is queryable or not is not relevant to the client. For client test coverage, we should simply check that the client properly propagates errors from the server to the user. This is not the right place to try exercising all possible server-side errors. That belongs in the server unit/integration tests instead.

vcrfxia · 2021-09-15T06:33:55Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

+  }
+
+  @Test
+  public void shouldPushQueryTableAfterInsert() throws Exception {


The existing test shouldInsertInto() already issues a push query after an insertInto() call. What additional coverage does this test buy us?

shouldInsertInto insert into a stream EMPTY_TEST_STREAM , this one insertInto a table.

If that's the purpose of this new test, can we rename the test to shouldInsertIntoTable() or similar, in order to better capture what the test is for?

vcrfxia · 2021-09-15T06:37:36Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

+  }
+
+  @Test
+  public void shouldPullQueryCTASTableAfterInsert() throws Exception {


+1 to Matthias: I don't think this test makes sense in this file. This test file is meant to test client-specific behavior. If the purpose of this test is to verify that INSERT VALUES does not update pull query state stores, then the test does not belong in this file. (In general, integration tests are (relatively) expensive so I'd err against adding redundant/duplicate tests.)

vcrfxia · 2021-09-15T06:40:16Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

+  }
+
+  @Test
+  public void shouldPullQueryCTASTableAfterInsertStream() throws Exception {


Ditto to my comment above -- I'm not convinced this test belongs in this file. I definitely think we should have a test that verifies that INSERT VALUES is allowed for tables, but I don't think we need to test auxiliary behaviors.

vcrfxia · 2021-09-15T06:42:08Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

@@ -657,22 +678,103 @@ public void shouldInsertInto() throws Exception {
  }

  @Test
-  public void shouldHandleErrorResponseFromInsertInto() {


This is important test coverage (that error responses are properly exposed to the user). If we are removing the specific cause of the error used in the existing test, then let's replace the error with a different error thrown from the endpoint, rather than removing the test entirely.

As mentioned above, I can create separate PR to add more tests catching different exceptions

As above, I don't think we should be removing test coverage in this PR. Let me know if what I'm saying isn't making sense, I'm happy to sync offline.

vcrfxia · 2021-09-15T06:42:23Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

@@ -858,25 +975,12 @@ public void shouldStreamInserts() throws Exception {
    assertThat(acksPublisher.isFailed(), is(false));
  }

-  @Test
-  public void shouldHandleErrorResponseFromStreamInserts() {


As above: this is important test coverage (that error responses are properly exposed to the user). If we are removing the specific cause of the error used in the existing test, then let's replace the error with a different error thrown from the endpoint, rather than removing the test entirely.

Mentioned above, I can add more case in different PR

As above: #8114 (comment)

lihaosky · 2021-09-15T21:18:39Z

Synced with @vcrfxia offline. Will

Add insert exception handling back in unit test
Remove unnecessary test in integration test

Thanks @mjsax and @vcrfxia for reviewing

vcrfxia

Thanks @lihaosky -- LGTM besides the minor comment inline.

vcrfxia · 2021-09-16T04:30:24Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

    );

    // Then
    assertThat(e.getCause(), instanceOf(KsqlClientException.class));
    assertThat(e.getCause().getMessage(), containsString("Received 400 response from server"));
-    assertThat(e.getCause().getMessage(), containsString("Cannot insert into a table"));
+    assertThat(e.getCause().getMessage(), containsString("Cannot insert values into an unknown stream"));


Let's update the error message (in the actual server endpoint) so that it says "unknown stream/table" rather than "unknown stream".

vcrfxia · 2021-09-16T18:03:09Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/api/impl/InsertsStreamEndpoint.java

@@ -89,7 +86,8 @@ private DataSource getDataSource(
    final DataSource dataSource = metaStore.getSource(sourceName);
    if (dataSource == null) {
      throw new KsqlApiException(
-          "Cannot insert values into an unknown stream: " + sourceName, ERROR_CODE_BAD_STATEMENT);
+          "Cannot insert values into an unknown stream/table: " + sourceName,
+            ERROR_CODE_BAD_STATEMENT);
    }

    if (dataSource.getKsqlTopic().getKeyFormat().isWindowed()) {


Can we also update the error message in this block to say stream/table rather than just stream?

vcrfxia

LGTM -- thanks @lihaosky !

lihaosky requested review from mjsax and a team September 8, 2021 23:39

lihaosky linked an issue Sep 8, 2021 that may be closed by this pull request

Inserting values into tables is allowed by the CLI, but not the Java API #7948

Closed

lihaosky marked this pull request as ready for review September 8, 2021 23:42

lihaosky force-pushed the 7948-fix branch 4 times, most recently from fa30ecc to 4481cbe Compare September 9, 2021 23:12

mjsax reviewed Sep 10, 2021

View reviewed changes

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java Outdated Show resolved Hide resolved

mjsax reviewed Sep 10, 2021

View reviewed changes

lihaosky force-pushed the 7948-fix branch from 4481cbe to 4d78ba2 Compare September 10, 2021 03:40

lihaosky commented Sep 10, 2021

View reviewed changes

lihaosky force-pushed the 7948-fix branch from 4d78ba2 to bc4e5ae Compare September 14, 2021 04:48

mjsax approved these changes Sep 14, 2021

View reviewed changes

vcrfxia reviewed Sep 15, 2021

View reviewed changes

vcrfxia changed the title ~~fix: issue 7948. Allow insert into table using Java API~~ fix: allow insert into table using HTTP/2 endpoint and Java API Sep 15, 2021

lihaosky force-pushed the 7948-fix branch from bc4e5ae to 9cb0243 Compare September 15, 2021 22:15

vcrfxia reviewed Sep 16, 2021

View reviewed changes

lihaosky force-pushed the 7948-fix branch from 9cb0243 to d060472 Compare September 16, 2021 17:12

vcrfxia reviewed Sep 16, 2021

View reviewed changes

lihaosky force-pushed the 7948-fix branch from d060472 to 7d22e23 Compare September 16, 2021 18:05

vcrfxia approved these changes Sep 16, 2021

View reviewed changes

lihaosky force-pushed the 7948-fix branch from 7d22e23 to 6a8c630 Compare September 16, 2021 21:52

fix: issue 7948. Allow insert into table using Java API

43878fc

lihaosky force-pushed the 7948-fix branch from 6a8c630 to 43878fc Compare September 17, 2021 03:29

lihaosky merged commit a6c2cac into confluentinc:master Sep 17, 2021

lihaosky deleted the 7948-fix branch September 17, 2021 05:55

fix: allow insert into table using HTTP/2 endpoint and Java API #8114

fix: allow insert into table using HTTP/2 endpoint and Java API #8114

Conversation

lihaosky commented Sep 8, 2021 • edited by vcrfxia Loading

Description

Changes

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

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

mjsax commented Sep 10, 2021 • edited Loading

lihaosky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjsax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a 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

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

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

lihaosky commented Sep 15, 2021

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

lihaosky commented Sep 8, 2021 •

edited by vcrfxia

Loading

mjsax commented Sep 10, 2021 •

edited

Loading