Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8312 )
Change subject: KUDU-2191 (4/n): WIP: Hive Metastore catalog manager integration ...................................................................... Patch Set 3: (36 comments) http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/CMakeLists.txt File src/kudu/integration-tests/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/CMakeLists.txt@79 PS2, Line 79: ADD_KUDU_TEST(master_hms-itest) Nit: sort order. http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/master_hms-itest.cc@75 PS2, Line 75: e = true Maybe use ExternalMiniClusterITestBase? http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/master_hms-itest.cc@79 PS2, Line 79: ASSERT_OK(cluster_->Start()); Can this be omitted? http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/master_hms-itest.cc@144 PS2, Line 144: ASSERT_OK(hms_client_->GetAllTables(hms_database_name, &tables)); I think you can use ContainsKey here, from map-util.h. http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/master_hms-itest.cc@155 PS2, Line 155: } // namespace kudu ContainsKey here too. http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.h@33 PS2, Line 33: #include <boost/optional/optional.hpp> Not used? http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@231 PS2, Line 231: "Addresses of Hive Metastore"); Should probably beef this up a bit to explain what setting this actually does. http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@634 PS2, Line 634: CHECK_OK(HostPort::ParseStrings(FLAGS_hive_metastore_addresses, 9083, &addresses)); 9083 is the default HMS port? Probably deserves more visibility than inlined in catalog_manager.cc. Maybe as a constant in hms_client.h? Also, could you add a comment justifying the CHECK_OK? It's guaranteed because of the gflag validator, right? http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@1066 PS2, Line 1066: if (hms_catalog_) { The order of operations in CatalogManager::Shutdown is tricky and has bitten us before in various ways. How did you arrive at this precise placement of stopping hms_catalog? What constraints are there (if any) about where it should happen? http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@1415 PS2, Line 1415: // Create the table in the HMS. It's worth noting what happens to this table if step e fails. http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@1598 PS2, Line 1598: if (hms_catalog_) { Why is it OK if this succeeds but step 3 fails? I can take my answer in the form of a code comment. http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@2092 PS2, Line 2092: Status s = hms_catalog_->AlterTable(table->id(), table_name, new_name, new_schema); Shouldn't some aspect of this be conditioned on one (or several) of has_foo_changes? For example, if neither the table name nor the schema have changed, why send the alter to HMS? http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.h File src/kudu/master/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.h@29 PS3, Line 29: #include "kudu/hms/hms_client.h" Seems like you may be able to forward declare HmsClient. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.h@42 PS3, Line 42: struct Rpc { Maybe define this as a private nested class of HmsCatalog, to make it clear that public users of HmsCatalog aren't expected to use it? http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.h@51 PS3, Line 51: class HmsCatalog { Please doc the class and its methods. I'm especially interested in the synchronization, the reason for a dedicated thread, etc. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.h@70 PS3, Line 70: Schema Can you get away with forward declaring Schema? http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.h@90 PS3, Line 90: // Only mutated by the worker thread. Maybe it shouldn't be a class member then? It could be declared on the stack in Run(). http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.h@91 PS3, Line 91: boost::optional<hms::HmsClient> client_; Worth a comment explaining why this is optional. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc File src/kudu/master/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@31 PS3, Line 31: #include "kudu/master/catalog_manager.h" : #include "kudu/master/sys_catalog.h" Why do you need these? http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@34 PS3, Line 34: #include "kudu/util/flag_tags.h" Not used? http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@38 PS3, Line 38: using rapidjson::Document; : using rapidjson::Value; These aren't used. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@73 PS3, Line 73: worker = worker_.get(); Should this also null out worker_? What's the expected behavior of multiple Stop() calls? http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@97 PS3, Line 97: Synchronizer s; : rpc.callback = s.AsStdStatusCallback(); : : { : MutexLock lock(lock_); : CHECK(running_); : rpc_queue_.emplace_back(std::move(rpc)); : if (rpc_queue_.size() == 1) { : cond_.Signal(); : } : } : : return s.Wait(); Encapsulate this in a common method shared by CreateTable, DropTable, etc? http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@122 PS3, Line 122: return client->DropTableWithContext(hms_database, hms_table, env_ctx); Just curious, why doesn't dropping a table use a hive::Table as input? http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@131 PS3, Line 131: if (rpc_queue_.size() == 1) { : cond_.Signal(); : } Why bother checking the rpc_queue_ size? Isn't always signalling correct too, and simpler? http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@184 PS3, Line 184: string("TODO") Can you add a TODO comment explaining what's missing here? http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@191 PS3, Line 191: Status HmsCatalog::ParseTableName(const string& table, Would love to see a barrage of tests on this method. I think it can be static, so shouldn't be too hard to write tests for it. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@199 PS3, Line 199: a Maybe mention that there should be exactly one database/table separator. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@229 PS3, Line 229: client_ = hms::HmsClient(address); So we try to connect to the HMS on each address until we find one that works? Add a comment explaining this; on first glance I thought the code as written was buggy. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@237 PS3, Line 237: LOG(WARNING) << "Unable to connect to the Hive Metastore (" : << address.ToString() << "): " << s.ToString(); Can save some space with WARN_NOT_OK here. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@248 PS3, Line 248: MutexLock lock(lock_); : rpc_queue_.swap(rpcs); Nit: indentation. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@263 PS3, Line 263: lock Nit: stylistic, but I like to use really short variable names (like 'l') for lock guards, to reflect their short scope. In this case it's also a useful way to disambiguate between 'lock' and 'lock_', which are quite similar looking. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@264 PS3, Line 264: if (rpc_queue_.empty()) { Spurious wake-ups are possible; this should be a while rather than an if. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.cc@306 PS3, Line 306: // If it's an IO error, assume the connection is toast. What other interesting error classes would we expect to see? Why is IOError the only one that merits resetting the connection? http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/util/net/net_util.h@133 PS2, Line 133: bool ValidateAddressListFlag(const char* flag_name, const std::string& addr_list); > warning: function 'kudu::ValidateAddressListFlag' has a definition with dif Doc please. Would be good to explain that the somewhat odd signature (i.e. not returning a Status) is so that it can be used directly as a gflag validator. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/util/net/net_util.cc@279 PS3, Line 279: WARN_NOT_OK(s, Substitute("invalid flag $0", flag_name)); I think ERROR is a more appropriate loglevel. -- To view, visit http://gerrit.cloudera.org:8080/8312 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1b8f360408b05d6ac4f7359a8ac0dd889b196baf Gerrit-Change-Number: 8312 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 19 Oct 2017 00:20:30 +0000 Gerrit-HasComments: Yes
