Abhishek Chennaka 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 y The flaky failure is fixed and the tests have been enabled back. 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 The comment is no more. 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 follo Yes, once we have the initial server and client bits in place we can have a follow-up changelist to address the alter table behavior. 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 t Yes, that is being worked upon in the following patches involving c++ client. 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 Done 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_in Done 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- Done 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 kud Done 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 ch Done 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. Alte Done http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/auto_incrementing-itest.cc@158 PS16, Line 158: InsertData > ditto Done 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 Done http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/integration-tests/auto_incrementing-itest.cc@166 PS16, Line 166: ScanTablet > ditto Done 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 ch Done 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 Done -- 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 <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 10 Jan 2023 02:15:50 +0000 Gerrit-HasComments: Yes
