Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17399 )
Change subject: KUDU-3164: Add table comment support to Kudu ...................................................................... Patch Set 3: (6 comments) I did a quick first pass. Overall looks fine, a few questions and nits. http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/client/client.cc@928 PS3, Line 928: req.set_comment(data_->comment_.get()); What if this newer client runs against a server of prior version where comments are not yet supported? Should the client get back an error because the server doesn't support setting comments or we are OK with silently dropping the comment information on the floor and sending back all-is-well response? http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/client/client.cc@1035 PS3, Line 1035: const string& comment, nit: the order of the parameters seems a bit odd -- I'd expect the 'comment' to come after 'owner' instead of splitting 'schema' and 'partition_schema'. http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/master/catalog_manager.cc@191 PS3, Line 191: DEFINE_int32 Would -1 be a valid value for this flag? If not, maybe use uint32 type of add a validator? http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/master/catalog_manager.cc@1598 PS3, Line 1598: id nit: maybe, change this to 'comment'? http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/master/catalog_manager.cc@3055 PS3, Line 3055: nit: the indent if off http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/master/master-test.cc@1007 PS3, Line 1007: const char *kTableName nit: it might be constexpr const char* const kTableName -- To view, visit http://gerrit.cloudera.org:8080/17399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic4dbf4018c38fdd0c7f748f4dfc26816f22bd5b2 Gerrit-Change-Number: 17399 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 07 May 2021 18:42:00 +0000 Gerrit-HasComments: Yes
