Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8312 )
Change subject: KUDU-2191 (6/n): Hive Metastore catalog manager integration ...................................................................... Patch Set 9: (52 comments) http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/hms/hms_client.cc File src/kudu/hms/hms_client.cc: http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/hms/hms_client.cc@193 PS8, Line 193: // Also check that the HMS is configured to allow changing the type of : // columns, which is required to support dropping columns. > Could you elaborate on this a bit? It's not intuitive why it needs to be le Added more elaboration. tldr; hive is a real :dumpster_fire2: http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/hms/hms_client.cc@202 PS8, Line 202: if (boost::iequals(disallow_incompatible_column_type_changes, "true")) { > Hmm, is it really possible for the config's value to have a mixed case? Ask Yes, I believe it's taken directly from the hive-site.xml so it's possible to have mixed case. Line 185 is matching on a class name, and Java is case sensitive so it's not a valid configuration to screw that up. http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/hms/hms_client.cc@218 PS8, Line 218: bool HmsClient::IsConnected() { > Maybe add a bit of test coverage for this new method? Done http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/hms/mini_hms.cc@214 PS8, Line 214: <value>jdbc:derby:$1/metadb;create=true</value> > Why the change from 'memory' back to 'directory'? I changed it originally in the hope that it would make things a bit faster if derby didn't have to write to disk. It's being changed back because I've added tests that stop and restart the HMS, and if it's not on disk the HMS will come back with an empty DB. http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/CMakeLists.txt File src/kudu/integration-tests/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/CMakeLists.txt@87 PS8, Line 87: ADD_KUDU_TEST(master_hms-itest) > Could you run this new test with KUDU_MEASURE_TEST_CPU_CONSUMPTION=1 (see r Working on this. http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/integration-tests/master-stress-test.cc File src/kudu/integration-tests/master-stress-test.cc: http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/integration-tests/master-stress-test.cc@343 PS9, Line 343: return Substitute("default.table_$0", oid_generator_.Next()); > Not necessary yet? No, it's not but it doesn't hurt anything so I figured I may as well. http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@122 PS8, Line 122: > Could RETURN_NOT_OK instead. Done http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@162 PS8, Line 162: string table_name = Substitute("$0.$1", hms_database_name, hms_table_name); : : // Attempt to create the table before the database is created. : Status s = CreateTable(hms_database_name, hms_table > Could be its own test (i.e. TestCreateTableWithoutDatabase) I've tried to reduce the number of tests cases since spinning up an HMS is very expensive (~10 seconds), so having many short tests becomes really expensive. If you have an idea for how to fix that, or still feel like they should be split up I can go ahead, though. http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@190 PS8, Line 190: : // Shutdown the HMS and try to create a table. : ASSERT_OK(StopHms()); : : s = CreateTable(hms_database_name, "foo"); : ASSERT_TRUE(s.IsNetworkError()) << s.ToString(); : : // Start the HMS and try again. : ASSERT_OK(StartHms()); : ASSERT_OK(CreateTable(hms_database_name, "foo")); > Could break out into a separate test since it only shares HMS database stat Likewise. http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@207 PS8, Line 207: : // Create the database. : hive::Database db; : db.name = hms_database_name; : ASSERT_OK(hms_client_->CreateDatabase(db)); > Every test does this; consider doing it in the test fixture. Done http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@255 PS8, Line 255: hive::EnvironmentContext env_ctx; > Why is it necessary to provide the table's ID here, on L270, and on L371, b This wasn't really necessary. If you do provide the table ID when dropping a Kudu table, the Kudu HMS plugin will ensure the expected table ID matches the actual, but if you don't provide it the drop will still occur. http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@268 PS8, Line 268: : // Drop the HMS table entry, then c > I was confused when I first read this, but now I think I understand: it's l You may be reading too much into this. This is just setting up a hypothetical scenario in which the Kudu/HMS catalogs may become unsychronized. In this case we have a Kudu table, and an HMS table entry with the same name, but the HMS table entry isn't a Kudu table entry. If we rename the Kudu table, it shouldn't attempt to rename the HMS entry (since it's not for the Kudu table being renamed). http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@285 PS8, Line 285: hms_external_table_name, > It was an external table created on L274 correct. > but didn't it get renamed to hms_rename_table_name on L276? No, because the HMS catalog entry wasn't a Kudu table entry matching the Kudu table being renamed, Kudu simply created a new HMS entry instead of altering the old one. This is important to support so that Kudu can synchronize the HMS and Kudu catalogs by renaming Kudu tables to avoid collisions. http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@309 PS8, Line 309: hms_table.sd.cols.clear(); > After this step could you use CheckCatalogs (or a variant thereof) to asser I've done a one-off check since I don't think we check anything similar elsewhere. http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@339 PS8, Line 339: s = table_alterer->DropColumn("int32_val")->Alter(); > Why does this fail? The non-Kudu HMS table entry has the same database and No, one of the tenets of this integration is that we should _never_ modify a non-Kudu HMS entry. If we did that it could really really screw things up. This situation can be 'resolved' either using a manual tool which will offer to delete the HMS entry, or by renaming the Kudu table to avoid the name collision. http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@354 PS8, Line 354: > Maybe ensure it's gone from Kudu too, just to make sure that the HMS integr Done http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@374 PS8, Line 374: ASSERT_TRUE(s.IsNotFound()) << s.ToString(); > Likewise, make sure it was really dropped from Kudu. Done http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@384 PS8, Line 384: NO_FATALS(CheckCatalogs(hms_database_name, hms_table_name)); > Check that it's gone from both Kudu and HMS? Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/CMakeLists.txt File src/kudu/master/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/CMakeLists.txt@76 PS9, Line 76: ADD_KUDU_TEST(hms_catalog-test) > Likewise, please assess the perf impact of this test and annotate it accord Ack http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/catalog_manager.cc@697 PS9, Line 697: CHECK_OK(HostPort::ParseStrings(FLAGS_hive_metastore_addresses, > Since you're inside Init, may as well convert these CHECK_OKs to RETURN_NOT Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/catalog_manager.cc@1548 PS9, Line 1548: // Create the table in the HMS. > Can you expand this comment a bit to explain why this should happen before Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/catalog_manager.cc@1743 PS9, Line 1743: // Drop the HMS table entry. > As in Create, could you expand this to explain why this should happen here, Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/catalog_manager.cc@1755 PS9, Line 1755: auto abort_hms = MakeScopedCleanup([&] { > What does it mean for a ScopedCleanup to return something? It's run on scop It's a hack that allows using the RETURN_NOT_OK_LOG macro inside the lambda body. The returned value is never actually used. If you think this too clever I can write it out manually. http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/catalog_manager.cc@2254 PS9, Line 2254: if (hms_catalog_ != nullptr && has_metadata_changes) { > Add a comment explaining why we do this here and not after the catalog writ Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/catalog_manager.cc@2272 PS9, Line 2272: RETURN_NOT_OK_LOG(SchemaFromPB(l.data().pb.schema(), &schema), WARNING, > See above comment re: returning a value from a ScopedCleanup. Same thing. http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog-test.cc File src/kudu/master/hms_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog-test.cc@38 PS9, Line 38: > Even though master_hms-test provides integration testing between CatalogMan I agree. Will work on that in future revision. http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.h File src/kudu/master/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.h@61 PS9, Line 61: // Create a new table entry in the HMS. > Doc that this method fails if HMS is unreachable? Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.h@66 PS9, Line 66: // Drops a table entry from the HMS, if it exists. > Nit: it would be good to use the same tense when describing the functionali Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.h@91 PS9, Line 91: Status Reconnect(); > Doc this? Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.h@103 PS9, Line 103: static Status ParseTableName(const std::string& table, > And this? Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.h@110 PS9, Line 110: gscoped_ptr<ThreadPool> threadpool_; > unique_ptr This doesn't appear to be possible due to the API of ThreadPoolBuilder. http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc File src/kudu/master/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@52 PS9, Line 52: const int HmsCatalog::kNumRetries = 1; > Do you think it would it be useful to convert this into an advanced gflag? Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@76 PS9, Line 76: .set_min_threads(0) > This is the default; it can be omitted. Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@77 PS9, Line 77: .set_max_threads(1) > Doc why capping at one thread is important. Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@95 PS9, Line 95: return Execute([=] (hms::HmsClient* client) { > Execute() waits for the provided functor to run, so why not capture variabl Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@162 PS9, Line 162: if (!s.ok()) { > I'm going to claim laziness and ask whether all of these edge cases are cov I believe that all edge-cases are covered by a combination of master_hms-itest and hms_catalog-test. http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@180 PS9, Line 180: // Handle all other errors. > Nit: not really "handling" per se; it's just that all other errors are fata Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@201 PS9, Line 201: If : // the original table object and the new table object match exactly then : // we don't need to alter the table in the HMS. > Does this short circuit make us vulnerable to a TOCTOU race, where we retur Well you're right to expect there are TOCTOU races going on all over the place here, but this isn't one of them. Since we're only accessing the HMS once during this short-circuit, there's no TOCTOU. However in the non-short-circuited case there are absolutely TOCTOU issues where we can overwrite a table entry in the HMS which has been altered in the meantime; the way we mostly kind of solve this is to have the Kudu HMS plugin check that the Kudu table ID matches when altering a Kudu table entry. That provides a defense against the master accidentally altering a non-Kudu table entry, which would be catastrophic. http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@222 PS9, Line 222: // TODO(Todd): wrapping this in a TRACE_EVENT scope and a LOG_IF_SLOW and such > Nit: Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@238 PS9, Line 238: // Since every task submitted to the (single thread) pool runs this, it's : // essentially a single iteration of a run loop which handles HMS client : // reconnection and task processing. > Do you think the pool should have a min_thread count of 1 so that we don't Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@253 PS9, Line 253: // network or IO fault (not an applicaiton level failure). The HMS client > application Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@258 PS9, Line 258: for (int i = 0; i <= kNumRetries; i++) { > Seems like kNumRetries should depend at least somewhat on the number of HMS Reconnect() handles connecting to the live HMS, if others are down. Note that the HMS is active/active, it's not like our master where you have to go to a certain one. http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@291 PS9, Line 291: // A fatal error occurred. Tear down the connection, and try again. We > Is it possible for reconnect to succeed, for the task to exhibit a fatal er Yes, this is possible because we can't really guarantee that we bucket errors 100% accurately into application (non-retriable) vs fatal (retriable). To make sure this doesn't loop forever we break after kNumRetries, which defaults to the low value of 1. http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@300 PS9, Line 300: return callback(s); > This passes in the last failure; should we instead pass in the first? I can't think of a reason it would make a difference, unless you are suggesting they somehow be combined? http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@316 PS9, Line 316: hms_client_ = hms::HmsClient(address, hms::HmsClientOptions()); > Nit: might consider making HmsClientOptions() the default value of the cons This is here because we're missing a bunch of flags that set HmsClient options, like enabling kerberos etc. Going to fix in next revision. http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@324 PS9, Line 324: LOG(WARNING) << "Failed to connect to Hive Metastore (" : << address.ToString() << "): " << s.ToString(); > More terse: Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@330 PS9, Line 330: return s; > If we can't connect to any of the HMSs, should we return the first failure? I can't think of a reason it would make a difference. http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@368 PS9, Line 368: return string(); > Hmm, so what would happen at runtime if we returned an empty string here? A Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@387 PS9, Line 387: table->tableType = "MANAGED_TABLE"; > Should this constant be given a little more visibility? What's its signific Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@420 PS9, Line 420: "When the Hive Metastore integration is enabled, Kudu table names must be a " > Nit: as a convention we should start a status message with a lower case let Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/mini-cluster/external_mini_cluster-test.cc File src/kudu/mini-cluster/external_mini_cluster-test.cc: http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/mini-cluster/external_mini_cluster-test.cc@19 PS9, Line 19: #include <ostream> > Revert? Done http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/util/net/net_util.cc@18 PS9, Line 18: #include <cstdint> > Nit: this belongs in the next group of includes. Done -- 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: 9 Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 28 Mar 2018 23:23:45 +0000 Gerrit-HasComments: Yes
