AHeise commented on a change in pull request #17888:
URL: https://github.com/apache/flink/pull/17888#discussion_r757592067
##########
File path:
flink-connectors/flink-connector-hbase-2.2/src/test/java/org/apache/flink/connector/hbase2/HBaseConnectorITCase.java
##########
@@ -250,13 +252,23 @@ public void testTableSink() throws Exception {
+ " FROM "
+ TEST_TABLE_1;
+ TableResult tableResult = tEnv.executeSql(query);
+
// wait to finish
- tEnv.executeSql(query).await();
+ tableResult.await();
+
+ assertEquals(
+ "Expected INSERT rowKind", RowKind.INSERT,
tableResult.collect().next().getKind());
// start a batch scan job to verify contents in HBase table
TableEnvironment batchEnv = TableEnvironment.create(batchSettings);
batchEnv.executeSql(table2DDL);
+ Table countTable =
+ batchEnv.sqlQuery("SELECT COUNT(h.rowkey) FROM " +
TEST_TABLE_2 + " AS h");
Review comment:
Is this query and assertion actually improving the debug information?
For me it looks like it's strictly reducing the information that we already
have: Right now we know that the result is truncated at some point in time.
With this additional assertion, it feels like we only know that we have fewer
records but can't assert where they are missing. So, it could actually make
investigation much harder for this test in the future.
##########
File path:
flink-connectors/flink-connector-hbase-2.2/src/test/java/org/apache/flink/connector/hbase2/HBaseConnectorITCase.java
##########
@@ -250,13 +252,23 @@ public void testTableSink() throws Exception {
+ " FROM "
+ TEST_TABLE_1;
+ TableResult tableResult = tEnv.executeSql(query);
+
// wait to finish
- tEnv.executeSql(query).await();
+ tableResult.await();
+
+ assertEquals(
+ "Expected INSERT rowKind", RowKind.INSERT,
tableResult.collect().next().getKind());
// start a batch scan job to verify contents in HBase table
TableEnvironment batchEnv = TableEnvironment.create(batchSettings);
batchEnv.executeSql(table2DDL);
+ Table countTable =
+ batchEnv.sqlQuery("SELECT COUNT(h.rowkey) FROM " +
TEST_TABLE_2 + " AS h");
+
+ assertEquals("Expected 8 rows", 8L,
countTable.execute().collect().next().getField(0));
Review comment:
I'm wondering if we can avoid hardcoding "8" by using `expected.size()`?
We would need to pull expected to the top of the method probably for that but
it would certainly ease maintaining the test when we want to add more records
in the future (for example when we extend HBase to support special cases).
##########
File path:
flink-connectors/flink-connector-hbase-2.2/src/test/java/org/apache/flink/connector/hbase2/HBaseConnectorITCase.java
##########
@@ -270,7 +282,13 @@ public void testTableSink() throws Exception {
+ "FROM "
+ TEST_TABLE_2
+ " AS h");
- List<Row> results =
CollectionUtil.iteratorToList(table.execute().collect());
+
+ TableResult tableResult2 = table.execute();
+ // wait to finish
+ tableResult2.await();
+
+ List<Row> results =
CollectionUtil.iteratorToList(tableResult2.collect());
Review comment:
Are you sure that `await` is actually improving the situation? What I
see from the code, `await` is simply waiting for the first result row to
appear. From the test failure, we know that there are 3 out of 8 rows there. So
I don't think that this is enough.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
