Adar Dembo 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 4: (10 comments) Just responding to your responses, will defer a rereview until the tests pass (it's been long enough since my first review that I've forgotten all of the new code). 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(fuzz-itest RUN_SERIAL true) > Nit: sort order. I think you missed this one. 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: op()); > Maybe use ExternalMiniClusterITestBase? Done http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/master_hms-itest.cc@79 PS2, Line 79: Status StopHms() { > Can this be omitted? Done http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/master_hms-itest.cc@112 PS2, Line 112: b.AddColumn("decimal32_val")->Type(KuduColumnSchema::DECIMAL) > ??? The table_name() function expects a cref string, so std::move(table_name) has no effect. 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@1066 PS2, Line 1066: return s; > The order of operations in CatalogManager::Shutdown is tricky and has bitte I think you missed this one. http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@1598 PS2, Line 1598: } > Why is it OK if this succeeds but step 3 fails? I can take my answer in the Hmm, the new behavior treats HMS table entry dropping as fatal, so this new comment is no longer correct. http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@2092 PS2, Line 2092: case AlterTableRequestPB::DROP_RANGE_PARTITION: { > Shouldn't some aspect of this be conditioned on one (or several) of has_foo I think you missed this. http://gerrit.cloudera.org:8080/#/c/8312/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8312/4/src/kudu/master/catalog_manager.cc@1561 PS4, Line 1561: // TODO(dan): figure out how to test this. You can inject faults into SysCatalogTable::SyncWrite. Maybe that will help? http://gerrit.cloudera.org:8080/#/c/8312/4/src/kudu/master/catalog_manager.cc@2253 PS4, Line 2253: // TODO(dan): figure out how to test this. See an earlier comment about injecting failures into syscatalog writes. 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/util/net/net_util.h" > optional seems to require the include in order to compile. Not using optional anymore? -- 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: 4 Gerrit-Owner: Dan Burkert <[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: Tue, 06 Mar 2018 19:48:33 +0000 Gerrit-HasComments: Yes
