Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19097 )
Change subject: KUDU-1945 Auto-Incrementing Column ...................................................................... Patch Set 7: (24 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 ta Extended the test to read from a follower replica too. Let me know if that is sufficient. http://gerrit.cloudera.org:8080/#/c/19097/3//COMMIT_MSG@13 PS3, Line 13: f tables with : just one tablet, a > What is a single partitioned table? Tables with just one tablet. I'll elaborate in the commit message. http://gerrit.cloudera.org:8080/#/c/19097/3//COMMIT_MSG@21 PS3, Line 21: followe > followers Done 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: _conns = 0; > nit: what's the term for such columns? Is that auto-incremented, auto-incr Yes, let's go with auto-incrementing and keep it consistent. http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc@9605 PS3, Line 9605: // restarting. The result row count should match the number of total rows : // written by the client. > nit: add space after commas Done http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc@9611 PS3, Line 9611: d > nit: add period in the end of the sentence Done http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc@9611 PS3, Line 9611: [](KuduScanne > auto-incrementing? Done http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc@9629 PS3, Line 9629: 64_t key = 0; > nit: fix this to be style-compliant Done http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/client-test.cc@9630 PS3, Line 9630: > nit: add space after the comma Done 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: KuduColumnTy > nit: maybe, rename into is_autoincrement to follow the 'is_nullable' and 'i Updated it to is_auto_incrementing and placed it after is_immutable http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/client/schema.h@492 PS3, Line 492: { > How about: Done 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 auto-incrementing. > Whether the column is auto-incrementing Done http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/common/common.proto@144 PS3, Line 144: optional bool is_auto_incrementing = 14 [default = false]; > Add the default value to be more explicit. Done 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: is_auto_increm > nit: consider naming this 'is_auto_incremented' or 'is_auto_incrementing' t Done http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/common/schema.h@478 PS3, Line 478: string comment_ > nit: rename into is_auto_incremented_ or is_auto_incremetning_ Have set it to is_auto_incrementing_ 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, : bool is_auto_incrementing, : const void* read_default, : const void* write_def > nit: why not to update AddColumn() method above, adding extra parameter? The idea was to move this higher after is_immutable later on (next patch revision) and hence this approach. 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_incrementing_col_idx = > Should there be 'break' out of the cycle since there can be only one auto-i Yes, that makes sense. Updated it. The followup change-lists are expected to get rid of this section. 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: optional NoOpRequestPB noop_request = 999; > I guess it makes sense adding a separate message to contain information rel Yes it makes sense to add a new message in WriteRequestPB about the auto increment column http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/consensus/consensus.proto@213 PS3, Line 213: > Maybe, use uint64? Unsigned integers do overflow in a more predictable way Done http://gerrit.cloudera.org:8080/#/c/19097/3/src/kudu/consensus/consensus.proto@213 PS3, Line 213: > Consider renaming into 'auto_increment_counter', 'auto_increment_value', or Done 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: write_req->CopyFrom(*state()->request()); : if (state_->tablet_replica()->t > What if the tablet has no auto-incremented column? Why to set this field a Makes sense. Done. 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: er(uint64_t au > Why to pass 64-bit integers as constant references? Referred this: https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs Now passed it by value. 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 by the prepare thread. > Can be updated only by the prepare thread. Done 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@70 PS3, Line 70: > What's the purpose of outputting this? That was for internal testing. Removed it. -- 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: Marton Greber <[email protected]> Gerrit-Comment-Date: Fri, 18 Nov 2022 16:30:57 +0000 Gerrit-HasComments: Yes
