Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10581 )
Change subject: KUDU-2191: Add a method to decide the scope of AlterTable ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/client.h@45 PS4, Line 45: #include "kudu/master/master.pb.h" This can't be included here; it's not part of the public client headers (and shouldn't be, since it's an internal header). http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/client.h@1194 PS4, Line 1194: KuduTableAlterer* system(master::AlterTableRequestPB::SystemType system); You can't use a PB-defined enum here. That's why my original suggestion differentiated between an enum (used in the wire protocol, where we must maintain ABI compatibility) and a bool (used here in the private API, which we can change at will). http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/table_alterer-internal.h File src/kudu/client/table_alterer-internal.h: http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/table_alterer-internal.h@82 PS4, Line 82: master::AlterTableRequestPB::NO_HMS; Shouldn't the default be ALL_SYSTEMS? http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/integration-tests/master_hms-itest.cc@48 PS4, Line 48: // Note: this test needs to be in the client namespace in order for You don't need to do this. See how ClientStressTest is used in FRIEND_TEST in KuduClient for an example. http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/integration-tests/master_hms-itest.cc@360 PS4, Line 360: bool exist; Nit: exists http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/catalog_manager.cc@2158 PS4, Line 2158: req.system() == master::AlterTableRequestPB::ALL_SYSTEMS) { To make this more future-proof, change the condition to != NO_HMS. That way if we add a third entry that disables some other external connector, this still works. http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto@570 PS4, Line 570: enum SystemType { Ni:t how about ExternalSystemType:? http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto@571 PS4, Line 571: ALL_SYSTEMS = 0; Nit: just ALL is enough, given that to use the enum value you need to qualify it with SystemType anyway. http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto@574 PS4, Line 574: optional SystemType system = 5 [ default = ALL_SYSTEMS ]; Nit: how about system_to_alter. -- To view, visit http://gerrit.cloudera.org:8080/10581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678 Gerrit-Change-Number: 10581 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 08 Jun 2018 03:39:52 +0000 Gerrit-HasComments: Yes
