Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19949 )
Change subject: KUDU-3483 Fix flushing data in batch when table schema changed ...................................................................... Patch Set 16: Code-Review+1 (6 comments) http://gerrit.cloudera.org:8080/#/c/19949/16/java/kudu-client/src/test/java/org/TestSchema.java File java/kudu-client/src/test/java/org/TestSchema.java: http://gerrit.cloudera.org:8080/#/c/19949/16/java/kudu-client/src/test/java/org/TestSchema.java@110 PS16, Line 110: // Two schemas have different type of columns. : assertFalse(schema4.equals(schema6)); According to the comments, I guess you want to compare schema6 with schema5? http://gerrit.cloudera.org:8080/#/c/19949/16/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java: http://gerrit.cloudera.org:8080/#/c/19949/16/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@143 PS16, Line 143: while (it.hasNext()) { : it.next(); : ++rowCount; : } nit: You can count the rows by it.getNumRows(). http://gerrit.cloudera.org:8080/#/c/19949/16/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@176 PS16, Line 176: Arrays.toString(rowErrors)), 0, rowErrors.length); Also to check the actual row count wrote into Kudu equals to 2? http://gerrit.cloudera.org:8080/#/c/19949/16/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@181 PS16, Line 181: data nit: row? The same to other places. http://gerrit.cloudera.org:8080/#/c/19949/16/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@188 PS16, Line 188: 3 Is it enough to add just 1 column simply? http://gerrit.cloudera.org:8080/#/c/19949/16/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@207 PS16, Line 207: session.flush(); I guess this test case is to verify the flush() work well even if the buffered ops are in different schemas, so how about checking the actually flushed row count is 2? The same to other places. -- To view, visit http://gerrit.cloudera.org:8080/19949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6501962b32814d121f180b2942999c402d927db Gerrit-Change-Number: 19949 Gerrit-PatchSet: 16 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Wed, 18 Oct 2023 08:40:53 +0000 Gerrit-HasComments: Yes
