Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19097 )
Change subject: KUDU-1945 Auto-Incrementing Column ...................................................................... Patch Set 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/19097/3//COMMIT_MSG Commit Message: PS3: > Extended the test to read from a follower replica too. Let me know if that I see that's something about reading from a replica that elected a leader after restart. A couple things here: * the new leader might be the same replica as before the restart, at least sometimes? * only a majority of replicas is needed to elect a new leader, but the rest might suffer from some issue which might left hidden if reading only from a single replica from the majority that elected a new leader Overall, reading only from current leader (even if that's not the same replica it was when writing) could leave some issues undetectable. I'd vote to update the test to read from all the available replicas. 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 ? 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 ? 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: (1) this file is a part of public API, and we need to keep new versions backwards-compatible, so I'm not sure changing the values for already existing enumeration items is acceptable; if adding new elements into the enum, just add append them to the list and give it next value (2) it's better to separate this change into its own patch (3) just a thought: as an additional alias to UINT64, maybe we should calls the type SERIAL (or similar) as well? 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. 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 return Status::InvalidArgument() in this case : the client didn't send any values for the auto-incrementing column, that's rather an internal counter reached some prohibitively high value. What if simply allow for overflow to happen without marking the operation as failed? >From the other side, if marking an op as failed, does it mean no more rows can >be inserted into the table even if it's empty as of now? 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 of the counter to be an non-operable state. Otherwise, next write will be successful even if the counter has overflown. 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 guess that does not make much sense, but I don't see where that's verified. 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? 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 instead -- 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-Comment-Date: Sat, 19 Nov 2022 03:46:35 +0000 Gerrit-HasComments: Yes
