Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20083 )
Change subject: [client] KUDU-1945 Add upsert support ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/20083/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20083/1//COMMIT_MSG@9 PS1, Line 9: c++ nit: C++ http://gerrit.cloudera.org:8080/#/c/20083/1//COMMIT_MSG@10 PS1, Line 10: tions in place. nit: missing words or a typo? http://gerrit.cloudera.org:8080/#/c/20083/2//COMMIT_MSG Commit Message: PS2: I think at least one piece is missing in this patch: client-side verification of write operations to be sent to the server side, i.e. making sure that the auto-incrementing column isn't set for operations that would be rejected at the server side. Adding client-side verification of such sort would save traffic and CPU cycles at the server side. http://gerrit.cloudera.org:8080/#/c/20083/2//COMMIT_MSG@7 PS2, Line 7: upsert nit: UPSERT http://gerrit.cloudera.org:8080/#/c/20083/2//COMMIT_MSG@9 PS2, Line 9: upsert UPSERT http://gerrit.cloudera.org:8080/#/c/20083/2//COMMIT_MSG@9 PS2, Line 9: c++ C++ http://gerrit.cloudera.org:8080/#/c/20083/2//COMMIT_MSG@10 PS2, Line 10: previoulsy typo? http://gerrit.cloudera.org:8080/#/c/20083/2//COMMIT_MSG@10 PS2, Line 10: upsert UPSERT http://gerrit.cloudera.org:8080/#/c/20083/2//COMMIT_MSG@11 PS2, Line 11: only supports if the entire primary key some words lost or some typos present? http://gerrit.cloudera.org:8080/#/c/20083/1/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/20083/1/src/kudu/common/schema.cc@314 PS1, Line 314: key_columns It would be great to add a comment to clarify why key_columns is essential in this check. http://gerrit.cloudera.org:8080/#/c/20083/2/src/kudu/integration-tests/auto_incrementing-itest.cc File src/kudu/integration-tests/auto_incrementing-itest.cc: http://gerrit.cloudera.org:8080/#/c/20083/2/src/kudu/integration-tests/auto_incrementing-itest.cc@292 PS2, Line 292: Consider adding test cases to cover error conditions as well. -- To view, visit http://gerrit.cloudera.org:8080/20083 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27a95e3a6b1d1b584cad849978313b3c8222cd3d Gerrit-Change-Number: 20083 Gerrit-PatchSet: 2 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, 16 Jun 2023 18:38:59 +0000 Gerrit-HasComments: Yes
