Hao Hao 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 5: (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/util/kudu_export.h" > This can't be included here; it's not part of the public client headers (an Done http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/client.h@1194 PS4, Line 1194: KuduTableAlterer* system(bool kudu_only); > You can't use a PB-defined enum here. That's why my original suggestion dif Done 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::ALL_SYSTEMS; > Shouldn't the default be ALL_SYSTEMS? Oops, my bad. 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: using client::KuduColumnSchema; > You don't need to do this. See how ClientStressTest is used in FRIEND_TEST Done http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/integration-tests/master_hms-itest.cc@360 PS4, Line 360: ASSERT_OK(hms_client_->GetTable("default", "a", &hms_table)); > Nit: exists Done 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_to_alter() != master::AlterTableRequestPB::NO_HMS) { > To make this more future-proof, change the condition to != NO_HMS. That way Done 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 ExternalSystemType { > Ni:t how about ExternalSystemType:? Done 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 quali Hmm, I used ALL_SYSTEMS because it seems SystemType is not needed to refer the enum value. In the proto buff auto-generated code it has 'static const SystemType ALL_SYSTEMS = AlterTableRequestPB_SystemType_ALL_SYSTEMS;' http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto@574 PS4, Line 574: optional ExternalSystemType system_to_alter = 5 [ default = ALL_SYSTEMS ]; > Nit: how about system_to_alter. Done -- 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: 5 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: Mon, 11 Jun 2018 19:42:27 +0000 Gerrit-HasComments: Yes
