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 13: (5 comments) http://gerrit.cloudera.org:8080/#/c/15841/12/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/15841/12/src/kudu/integration-tests/master_hms-itest.cc@303 PS12, Line 303: TEST_F(MasterHmsTest, TestAlterTableOwner) { > Would it also be possible to test the case where we're adding an owner to a Do you mean the empty string case in kudu-tool-test? That one was meant to test this upgrade scenario, why do you think it's different? http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/catalog_manager.cc@2522 PS10, Line 2522: } : > Whoops, indeed :) Sure, it's valid, you can modify both in a single alter. http://gerrit.cloudera.org:8080/#/c/15841/12/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/15841/12/src/kudu/master/catalog_manager.cc@187 PS12, Line 187: n." > nit: unrelated, but could you add a period here? Done http://gerrit.cloudera.org:8080/#/c/15841/12/src/kudu/master/catalog_manager.cc@2684 PS12, Line 2684: > nit: could you move this closer to where it's used so readers don't need lo actually it's used only once so I removed it. http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/hms_notification_log_listener.cc File src/kudu/master/hms_notification_log_listener.cc: http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/hms_notification_log_listener.cc@385 PS10, Line 385: before_table.owner != after_table.owner > Great, thanks for the clarification. It should in the client side, but the server side would return an error as we have an emptyness check (which can be turned off for tests with a hidden flag). -- 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: 13 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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, 08 Jul 2020 12:16:50 +0000 Gerrit-HasComments: Yes
