Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19097 )
Change subject: KUDU-1945 Auto-Incrementing Column ...................................................................... Patch Set 7: (10 comments) Will create a new test case under integrated_tests to scan all the replicas in future revisions of this patch. http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/client-test.cc@9866 PS7, Line 9866: ASSERT_OK(ScanToStrings(&scanner, &rows)); > Does it make sense to add an assert on rows.size() == kNumRows ? Done http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/client-test.cc@9882 PS7, Line 9882: ASSERT_OK(ScanToStrings(&scanner, &rows)); > Does it make sense to add an assert on rows.size() == kNumRows ? Done http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/schema.h@223 PS7, Line 223: UINT64 = 4, : STRING = 5, : BOOL = 6, : FLOAT = 7, : DOUBLE = 8, : BINARY = 9, : UNIXTIME_MICROS = 10, : DECIMAL = 11, : VARCHAR = 12, : TIMESTAMP = UNIXTIME_MICROS, //!< deprecated, use UNIXTIME_MICROS : DATE = 13 > A couple of things here: Thanks for pointing that out. Posted a new patch for this. http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/schema.cc@477 PS7, Line 477: data_->auto_incrementing > Should we also check internal_type and nullable if data_->auto_incrementing Done http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/common/row_operations.cc@495 PS7, Line 495: INT64_MAX > I guess this value is different now since switching to uint64. Thanks for the catch, fixed it. http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/common/row_operations.cc@496 PS7, Line 496: Status::InvalidArgument("max auto-incrementing value reached") > Status::IllegalState() seems to fit better here unless there is required to We should not be allowing overflow of the counter and the reason being - if a user upserts UINT64_MAX number of rows, the next upserted row with the same primary key would update the existing row and not create a new row. The user expects UINT64_MAX+1 rows to be present but there are only UINT64_MAX rows present. So the tablet should not be receiving any writes. We can probably have an unsafe CLI tool to reset this counter in a separate changelist. http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/common/row_operations.cc@497 PS7, Line 497: // TODO(achennaka): return? : } : (*auto_incrementing_counter)++; : memcpy(tablet_row.mutable_cell_ptr(tablet_col_idx), auto_incrementing_counter, 8); : BitmapChange(tablet_isset_bitmap, client_col_idx, true); > Well, at this this shouldn't be done if interpreting maximum possible value True, I think it makes sense to return in line 497. http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/common/schema.h@241 PS7, Line 241: const void* write_default = nullptr, > BTW, do we allow setting write_default for an auto-incrementing column? I Added the verification in schema.cc http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/tablet/ops/write_op.cc File src/kudu/tablet/ops/write_op.cc: http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/tablet/ops/write_op.cc@160 PS7, Line 160: GetAutoIncrementingCounter() != 0 > What if counter starts with 0 or overflown and its current value is 0? Right, fixed the logic. http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/tserver/tserver.proto@142 PS7, Line 142: required > To allow for easier migration to proto3 consider using 'optional' here inst 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: 7 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: Thu, 08 Dec 2022 06:54:39 +0000 Gerrit-HasComments: Yes
