Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15841 )
Change subject: KUDU-3090: Native owner metadata in Kudu ...................................................................... Patch Set 7: (13 comments) 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 poss Done 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: set_ow > nit: just so it's clear that this is a request to change owners, let's foll Done 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: kOwner, > nit: remove spacing Done 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? Done 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: user_ > nit: snake_case for variable names Done 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: if (user && !req.has_owner()) { : req.set_owner(*user); > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/master/catalog_manager.cc@1517 PS6, Line 1517: D > nit: should DCHECK_NE(none, user) Done http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/master/catalog_manager.cc@2655 PS6, Line 2655: > nit: add a period Done http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/master/catalog_manager.cc@2676 PS6, Line 2676: > nit: update this and below Done http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/master/catalog_manager.cc@2823 PS6, Line 2823: table write above. > nit: this is redundant nice catch! 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: kUsername)); > Should this be kUsername? or none? Why an empty string? This table is supposed to be fixed by --create_missing_hms_tables --nofix_inconsistent_tables and --fix_inconsistent_tables is the one that changes the owner, so I put an empty string there as "hms fix" created new tables with no owner (with none it would create the table as the logged in user). Now that I thought about it some more I think it actually makes sense to create a new HMS table with the proper owner even with --nofix_inconsistent_tables as there's a difference between not fixing an inconsistent table or creating a table with inconsistent metadata, so I changed it and we can use kUsername here. I'm leaving this open for now in case you think we shouldn't change this behavior. http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/tools/kudu-tool-test.cc@4074 PS6, Line 4074: no > nit: not your fault, but seems this file has a few too many spaces in a few Done http://gerrit.cloudera.org:8080/#/c/15841/6/src/kudu/tools/kudu-tool-test.cc@4079 PS6, Line 4079: static_cast<string>("") > Same here? In this case we actually want to test that when we have an existing table with no owner in Kudu and run hms fix with --fix_inconsistent_tables the Kudu owner is updated to match HMS owner, so we need to use an empty string. -- 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: 7 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 01 Jul 2020 10:27:09 +0000 Gerrit-HasComments: Yes
