Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/17399 )
Change subject: KUDU-3164: Add table comment support to Kudu ...................................................................... Patch Set 4: (6 comments) 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 comm I think silently dropping is probably okay given that is how we have handled new table fields so far and that is the effective behavior for the HMS sync today without the field. http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/client/client.cc@1035 PS3, Line 1035: const KuduSchema& sche > nit: the order of the parameters seems a bit odd -- I'd expect the 'comment Done 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 I suppose not. There are a ton of those cases in this file. Perhaps it could be a follow up to address adding a bunch of validators for all these flags. http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/master/catalog_manager.cc@1598 PS3, Line 1598: co > nit: maybe, change this to 'comment'? Done http://gerrit.cloudera.org:8080/#/c/17399/3/src/kudu/master/catalog_manager.cc@3055 PS3, Line 3055: RETU > nit: the indent if off Done 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: constexpr const char* > nit: it might be constexpr const char* const kTableName Done -- 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: 4 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 08 May 2021 03:23:05 +0000 Gerrit-HasComments: Yes
