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 4: (7 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(fuzz-itest RUN_SERIAL true) > I think you missed this one. Done 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; > I think you missed this one. I just did it in reverse order of Init(), I'm not aware of any ordering issues per se, as long as Shutdown() isn't called concurrently with any other use of the hms catalog. http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@1598 PS2, Line 1598: } > Hmm, the new behavior treats HMS table entry dropping as fatal, so this new Done http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@2092 PS2, Line 2092: case AlterTableRequestPB::DROP_RANGE_PARTITION: { > I think you missed this. Done 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 That's a good idea, I'm going to leave these here for now until I can write some fault tests. Until then, I've changed MasterFailoverTest to have the HMS enabled and it does appear to be triggering these codepaths. 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" > Not using optional anymore? right, but HmsClient is now a field so I don't think it can be forward declared. http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.h@51 PS3, Line 51: ~HmsCatalog(); > Please doc the class and its methods. I'm especially interested in the sync I've documented the public api in the .h, there are already pretty extensive notes in the .cc about synchronization, retry behavior, etc. -- 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 <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: Thu, 08 Mar 2018 00:47:12 +0000 Gerrit-HasComments: Yes
