kudu-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Wong (Code Review)" <ger...@cloudera.org>
Subject [kudu-CR] KUDU-3090: Native owner metadata in Kudu
Date Wed, 01 Jul 2020 05:29:27 GMT
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 <granthenke@apache.org>
Gerrit-Reviewer: Andrew Wong <awong@cloudera.com>
Gerrit-Reviewer: Attila Bukor <abukor@apache.org>
Gerrit-Reviewer: Grant Henke <granthenke@apache.org>
Gerrit-Reviewer: Hao Hao <hao.hao@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Jul 2020 05:29:27 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message