Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19097 )
Change subject: KUDU-1945 Auto-Incrementing Column ...................................................................... Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/client/client-test.cc@9618 PS8, Line 9618: NewInsert What about UPSERT operation? How the auto-incrementing column should be specified for UPSERTs? http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/client/client-test.cc@9638 PS8, Line 9638: } > add some negative test cases, like set value for auto_incrementing_id colum +1 http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/common/row_operations.cc@498 PS8, Line 498: max auto-incrementing column value reached style nit: create a constexpr for this message and use in both Status object, or even have a static const Status to use in both places http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/common/row_operations.cc@500 PS8, Line 500: (*auto_incrementing_counter)++; Since there might be many columns encoded in a write operation PB, the counter might be incremented even if returning an error back due to some issues with rest of the columns. Do we want to increment the counter regardless due to simplicity of other reasons, or it make sense to set the output parameter 'auto_incrementing_counter' only if DecodedRowOperation::result turns to be Status::OK() just before returning out of this method? Would be great to add a comment to be explicit about our intentions here. http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tablet/tablet_auto_incrementing-test.cc File src/kudu/tablet/tablet_auto_incrementing-test.cc: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tablet/tablet_auto_incrementing-test.cc@80 PS8, Line 80: + style nit: add spaces around '+' http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tablet/tablet_replica.h@44 PS8, Line 44: #include "kudu/tablet/tablet.h" // IWYU pragma: keep Why to keep this header file added and also keeping the forward declaration of 'class Tablet' at line 93? It should be either of those, but not both, I guess? http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tserver/tserver.proto@140 PS8, Line 140: nit: remove this empty extra line? http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tserver/tserver.proto@144 PS8, Line 144: // A batched set of insert/mutate requests. nit: add an empty line to separate different messages -- 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: 8 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, 13 Dec 2022 07:44:57 +0000 Gerrit-HasComments: Yes
