Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19097 )
Change subject: [WIP] KUDU-1945 Auto-Increment Column ...................................................................... Patch Set 3: (25 comments) http://gerrit.cloudera.org:8080/#/c/19097/3//COMMIT_MSG Commit Message: PS3: Does it make sense to add a test to verify that all tablet replicas of a table with auto-incremeted column have same values in the auto-incremented column? http://gerrit.cloudera.org:8080/#/c/19097/3//COMMIT_MSG@13 PS3, Line 13: f a single : partitioned tables What is a single partitioned table? http://gerrit.cloudera.org:8080/#/c/19097/3//COMMIT_MSG@21 PS3, Line 21: clients followers http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc@9562 PS3, Line 9562: AutoIncrement nit: what's the term for such columns? Is that auto-incremented, auto-incrementing, auto-increment? Would be great to have this named consistency across the code. http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc@9605 PS3, Line 9605: .add_range_partition(lower0.release(),upper0.release()) : .add_range_partition(lower1.release(),upper1.release()) nit: add space after commas http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc@9611 PS3, Line 9611: auto increment auto-incrementing? http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc@9611 PS3, Line 9611: n nit: add period in the end of the sentence http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc@9629 PS3, Line 9629: ( int i = 0; i < rows.size(); i++ ) nit: fix this to be style-compliant http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc@9630 PS3, Line 9630: i, nit: add space after the comma http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/schema.h@351 PS3, Line 351: autoincrement nit: maybe, rename into is_autoincrement to follow the 'is_nullable' and 'is_immutable' pattern? Also, since the code is updated anyways, move this parameter to come after the 'is_immutable'? http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/schema.h@492 PS3, Line 492: Set the column to be an auto-incrementing column How about: Set the column to be auto-incrementing. http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/common/common.proto@143 PS3, Line 143: Whether the column is an auto-incrementing column Whether the column is auto-incrementing http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/common/common.proto@144 PS3, Line 144: optional bool is_auto_incrementing = 14; Add the default value to be more explicit. http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/common/schema.h@273 PS3, Line 273: auto_increment nit: consider naming this 'is_auto_incremented' or 'is_auto_incrementing' to be in-line with 'is_nullable' and 'is_immutable' http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/common/schema.h@478 PS3, Line 478: auto_increment_ nit: rename into is_auto_incremented_ or is_auto_incremetning_ http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/common/schema.h@1117 PS3, Line 1117: Status AddColumn(const std::string& name, : DataType type, : bool is_nullable, : bool is_immutable, : const void* read_default, : const void* write_default, : bool auto_increment); nit: why not to update AddColumn() method above, adding extra parameter? http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/common/schema.cc@304 PS3, Line 304: auto_increment_col_idx = i; Should there be 'break' out of the cycle since there can be only one auto-incrementing column in a table? http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/consensus/consensus.proto File src/kudu/consensus/consensus.proto: http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/consensus/consensus.proto@212 PS3, Line 212: // The auto increment column id for the first write op in the batch. I guess it makes sense adding a separate message to contain information related to auto-incremented columns in the table. Also, should this be moved into WriteRequestPB? http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/consensus/consensus.proto@213 PS3, Line 213: auto_increment_id Consider renaming into 'auto_increment_counter', 'auto_increment_value', or something like that: I guess the semantics behind the field is the starting value for the counter, but not an identifier per se. http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/consensus/consensus.proto@213 PS3, Line 213: int64 Maybe, use uint64? Unsigned integers do overflow in a more predictable way (just starting with 0), and using int64 isn't efficient for storing negative values (i.e. the second part of the range) per https://developers.google.com/protocol-buffers/docs/proto3#scalar http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/tablet/ops/write_op.cc File src/kudu/tablet/ops/write_op.cc: http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/tablet/ops/write_op.cc@159 PS3, Line 159: (*replicate_msg)->set_auto_increment_id(state_->tablet_replica()->tablet()-> : GetAutoIncrementCounter()); What if the tablet has no auto-incremented column? Why to set this field at all? http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/tablet/tablet.h@548 PS3, Line 548: const int64_t& Why to pass 64-bit integers as constant references? http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/tablet/tablet.h@809 PS3, Line 809: It is expected that this is only : // updated from a single thread, the prepare thread. Can be updated only by the prepare thread. http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/tablet/tablet_auto_increment-test.cc File src/kudu/tablet/tablet_auto_increment-test.cc: http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/tablet/tablet_auto_increment-test.cc@58 PS3, Line 58: TEST_F(AutoIncrementTabletTest, TestBasicInserts) { Would be great to have a comment to describe what exactly this scenario is trying to verify. http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/tablet/tablet_auto_increment-test.cc@70 PS3, Line 70: LOG(INFO) << out[i]; What's the purpose of outputting this? -- 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: 3 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Sat, 29 Oct 2022 02:48:46 +0000 Gerrit-HasComments: Yes
