Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15841 )
Change subject: KUDU-3090: Native owner metadata in Kudu ...................................................................... Patch Set 6: (13 comments) Are there non-HMS tests for ownership? http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/client/client.h@1051 PS6, Line 1051: Owner of the table. Should probably mention what this returns if this isn't set, if that's possible. Is it only empty when upgrading from older versions of Kudu? http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/client/table_alterer-internal.h File src/kudu/client/table_alterer-internal.h: http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/client/table_alterer-internal.h@77 PS6, Line 77: owner_ nit: just so it's clear that this is a request to change owners, let's follow suit with 'rename_to_' and call this 'set_owner_to_' or somesuch http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/hms/hms_catalog-test.cc File src/kudu/hms/hms_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/hms/hms_catalog-test.cc@315 PS6, Line 315: nit: remove spacing http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/integration-tests/hms_itest-base.cc File src/kudu/integration-tests/hms_itest-base.cc: http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/integration-tests/hms_itest-base.cc@214 PS6, Line 214: ASSERT_EQ(hms_table.owner, username); Should we also check that KuduTable::owner() is also set correctly here? http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/integration-tests/master_hms-itest.cc@309 PS6, Line 309: userA nit: snake_case for variable names http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/master/catalog_manager.cc@1500 PS6, Line 1500: const string& owner = req.has_owner() ? req.owner() : user.get(); : req.set_owner(owner); nit: spacing Also might be clearer as if (user && !req.has_owner()) { req.set_owner(*user); } http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/master/catalog_manager.cc@1517 PS6, Line 1517: R nit: should DCHECK_NE(none, user) http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/master/catalog_manager.cc@2655 PS6, Line 2655: 5 nit: add a period http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/master/catalog_manager.cc@2676 PS6, Line 2676: 6. nit: update this and below http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/master/catalog_manager.cc@2823 PS6, Line 2823: || req.has_new_table_owner() nit: this is redundant http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/tools/kudu-tool-test.cc@4067 PS6, Line 4067: static_cast<string>("") Should this be kUsername? or none? Why an empty string? http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/tools/kudu-tool-test.cc@4074 PS6, Line 4074: nit: not your fault, but seems this file has a few too many spaces in a few places http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/tools/kudu-tool-test.cc@4079 PS6, Line 4079: static_cast<string>("") Same here? -- To view, visit http://gerrit.cloudera.org:8080/15841 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I67f5bfdf56d409960365fd5803913a2d3800831d Gerrit-Change-Number: 15841 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 01 Jul 2020 05:29:27 +0000 Gerrit-HasComments: Yes
