Alexey Serbin 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 20: Code-Review+1 (23 comments) Thank you for the fix! Overall looks good to me, just a few nits and a request to extend the test coverage a bit (if it makes sense). http://gerrit.cloudera.org:8080/#/c/19949/20//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19949/20//COMMIT_MSG@11 PS20, Line 11: data nit: rows http://gerrit.cloudera.org:8080/#/c/19949/20//COMMIT_MSG@12 PS20, Line 12: these data nit: the data http://gerrit.cloudera.org:8080/#/c/19949/20//COMMIT_MSG@18 PS20, Line 18: outbound nit: out of bound http://gerrit.cloudera.org:8080/#/c/19949/20/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/20/java/kudu-client/src/test/java/org/TestSchema.java@97 PS20, Line 97: name of columns nit: column names http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/TestSchema.java@110 PS20, Line 110: type of columns. nit: column types http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/TestSchema.java@121 PS20, Line 121: } Could you also add a sub-scenario to test for the equality of two column schemas with exact the same types, names, sequence of columns but different nullability for a non-key column? http://gerrit.cloudera.org:8080/#/c/19949/20/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/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@138 PS20, Line 138: countRowCount nit: rename into countRows() ? http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@149 PS20, Line 149: public void testInsertDataWithChangedSchema() throws Exception { Could you add a comment for this new test, mentioning KUDU-3483 and clarifying what would be the behavior of this test if running this without the fix in AsyncKuduSession (I guess that would be array index out of bound exception, right)? http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@154 PS20, Line 154: AUTO_FLUSH_BACKGROUND Does it make sense to run this scenario with the MANUAL_FLUSH mode as well? http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@191 PS20, Line 191: table nit: the table http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@195 PS20, Line 195: new nit: the new http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@195 PS20, Line 195: data nit: row http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@214 PS20, Line 214: rename nit: 'renaming' or 'renamed' http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@217 PS20, Line 217: data nit: row http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@229 PS20, Line 229: table nit: the table http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@233 PS20, Line 233: new nit: the new http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@233 PS20, Line 233: data nit: row http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@249 PS20, Line 249: failed nit: failed to insert http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@255 PS20, Line 255: data nit: row http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@267 PS20, Line 267: table nit: the table http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@271 PS20, Line 271: data nit: row http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@271 PS20, Line 271: new nit: the new http://gerrit.cloudera.org:8080/#/c/19949/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@286 PS20, Line 286: failed nit: failed to insert -- 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: 20 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: Fri, 20 Oct 2023 17:37:01 +0000 Gerrit-HasComments: Yes
