Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19097 )
Change subject: KUDU-1945 Auto-Incrementing Column ...................................................................... Patch Set 16: (15 comments) http://gerrit.cloudera.org:8080/#/c/19097/16//COMMIT_MSG Commit Message: PS16: Could you add an explicit note that this functionality isn't ready to use yet at least due to the flakiness mentioned in ClientTestAutoIncrementingColumn.ReadAndWrite and in the AutoIncrementingItest.TestAutoIncrementingItestdisabled test which is disabled for a while? My concern is that the flakiness seems to be a real design issue and using current implementation as-is might lead to data inconsistencies and other side effects. http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/client/client-test.cc@9904 PS16, Line 9904: nit: add a comma after 'addressed' for readability http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/client/client-test.cc@9959 PS16, Line 9959: } Not sure it's supposed to be a part of this patch, but what about the following scenarios: * have a table without auto-incrementing column, and then alter the table, trying to adding such a column * have a table with auto-incrementing column, and then alter the table, trying to drop the column Should those fail or succeed? If we have checks in place, are they only client-side ones or we have proper validation for such cases at the server side as well? http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/client/schema.h@492 PS16, Line 492: ///@{ : /// Set the column to be auto-incrementing. : /// : /// Upon inserting rows to a table with an auto-incrementing column, values are : /// automatically assigned to field that are unique to the partition. This : /// makes such columns good candidates to include in a table's primary key. : /// : /// @note Column auto-incrementing may not be changed once a table is : /// created. : /// : /// @return Pointer to the modified object. : KuduColumnSpec* AutoIncrementing(); I pointed to an alternative to this approach in the counterpart patch for the Java client: https://gerrit.cloudera.org/#/c/19384/10/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@406 Does it make sense to apply similar approach here in the C++ client as well? The idea is to not add this AutoIncrementing() method here but introduce KuduAutoIncrementingColumnSpec builder that has only relevant methods. In addition, maybe introduce a new KuduSchemaBuilder::AddAutoIncrementingColumn() method to returns a pointer to a newly created KuduAutoIncrementingColumnSpec instance. If that sounds good to you, it could be done in a separate patch. However, that would allow to get rid of many unit tests and checks in the code, so implementing that approach in this changelist might be an option as well. What do you think? http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/common/row_operations.cc@498 PS16, Line 498: nit: the indent is off http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/common/schema.cc@669 PS16, Line 669: } Does it make sense to check for the proper type of the column if is_auto_incrementing == true? http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/auto_incrementing-itest.cc File src/kudu/integration-tests/auto_incrementing-itest.cc: http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/auto_incrementing-itest.cc@71 PS16, Line 71: using tserver::NewScanRequestPB; : using tserver::ScanResponsePB; : using tserver::ScanRequestPB; : using tserver::TabletServerServiceProxy; : using tablet::TabletReplica; nit: move these out to join the rest of the 'using' pack above in lines 49-62 ? http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/auto_incrementing-itest.cc@85 PS16, Line 85: kudu:: nit: I guess the 'kudu::' prefix could be removed since this is already kudu::itest namespace, no? http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/auto_incrementing-itest.cc@141 PS16, Line 141: CHECK_OK(SchemaToColumnPBs(schema, scan->mutable_projected_columns())); : CHECK_OK(cluster_->tserver_proxy(ts)->Scan(req, &resp, &rpc)); Instead of using CHECK_OK here, consider using RETURN_NOT_OK instead and change the return type of this method to Status. The rationale is to have test failed due to triggered assertion, but not SIGABRT if something goes south during the test. http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/auto_incrementing-itest.cc@157 PS16, Line 157: CreateTableWithData nit: wrap this into NO_FATALS to bail right away of an error happens. Alternatively, use RETURN_NOT_OK in the implementation of CreateTableWithData() an wrap it with ASSERT_OK here http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/auto_incrementing-itest.cc@158 PS16, Line 158: InsertData ditto http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/auto_incrementing-itest.cc@162 PS16, Line 162: auto * style nit: stick the asterisk to the type, not the variable http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/auto_incrementing-itest.cc@166 PS16, Line 166: ScanTablet ditto http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/raft_consensus-itest-base.cc File src/kudu/integration-tests/raft_consensus-itest-base.cc: http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/raft_consensus-itest-base.cc@25 PS16, Line 25: #include <type_traits> I guess you could remove this and have IWYU still happy since nothing is changed in the file, so it won't consider this to run its verification. http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/tablet/tablet_auto_incrementing-test.cc File src/kudu/tablet/tablet_auto_incrementing-test.cc: http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/tablet/tablet_auto_incrementing-test.cc@92 PS16, Line 92: ASSERT_EQ(s.ToString(), "Invalid argument: auto-incrementing column is incorrectly set"); nit for here and elsewhere in this file: the expected value comes first -- To view, visit http://gerrit.cloudera.org:8080/19097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dbde9095da78f6d1bd00adcc0a6e7dd63082bbc Gerrit-Change-Number: 19097 Gerrit-PatchSet: 16 Gerrit-Owner: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Thu, 05 Jan 2023 00:38:07 +0000 Gerrit-HasComments: Yes
