Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8313 )
Change subject: KUDU-2191 (10/n): Hive Metastore notification log event listener ...................................................................... Patch Set 4: (24 comments) http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/integration-tests/master_hms-itest.cc@183 PS3, Line 183: ASSERT_EQ(schema.Column(idx).name(), hms_table.sd.cols[idx].name); > Is this important? OpenTable() already verified the existence of the table, Done http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/integration-tests/master_hms-itest.cc@188 PS3, Line 188: ASSERT_EQ(hms::HmsClient::kKuduStorageHandler, > Nit: I think you're indenting two chars too much here and in the other ASSE Done http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/catalog_manager.h@520 PS3, Line 520: > Please update the method docs (for AlterTable too). Done http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/catalog_manager.h@527 PS3, Line 527: // but this function does not itself respond to the RPC. > Nit: maybe hms_notification_log_event_id so it's clear this is an HMS const Done http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/catalog_manager.cc@1576 PS3, Line 1576: } > warning: the parameter 'notification_log_event_id' is copied for each invoc Ignoring this, optional<int64_t> should be trivially copyable. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/catalog_manager.cc@1600 PS3, Line 1600: > Should include a comment about how if there's this ID, it means the request Done http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/catalog_manager.cc@1601 PS3, Line 1601: // i. Make the new table and tablets visible in the catalog. > std::move? I thought optionals should be moved, though maybe that depends o Yeah I don't think there's much point moving an integer type, but I've made the change because I think it might silence the tidy warning above. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.h File src/kudu/master/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.h@54 PS3, Line 54: > This dependency implies something about the create/destruct ordering of Hms This has changed significantly so I don't think it applies anymore. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc File src/kudu/master/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@270 PS3, Line 270: > You may want to have CatalogManager mediate this access, so it can fail whe I've retained the direct access of the sys catalog in the notification log listener, but added leadership locks. I'm not 100% sure how I've done it is correct, so if you think your original suggestion would be better I can switch over to that. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@306 PS3, Line 306: > What happened here? This no longer applies. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@343 PS3, Line 343: : : : : : > Why is this necessary? This will be checked by the CatalogManager in AlterT This no longer applies, although there is something like it still. We use it to determine causes of failure now. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@360 PS3, Line 360: > This was already warned on L356. Ack http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@361 PS3, Line 361: > What's preventing this from creating a tight loop, if GetNotificationEvents This no longer applies. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@373 PS3, Line 373: : : : : > I'm too lazy to check, but can eventType be switched on? No, it's a string. Thrift has proper enum support, so who knows why it was done this way. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@381 PS3, Line 381: : : > Can't you extract this from the status instead of relying on the ScopedLead Hmm, will you take a look at how it's done now? I retained the spirit of this implementation, but I think it would be good if you took a look at the current impl. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@421 PS3, Line 421: > Unused? Done http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@423 PS3, Line 423: > Might be nice to include a comment with a sample JSON representation of an The JSON events are super ugly. They have no whitespace, tons of extraneous fields, etc. I don't think it would help much, but if you still want I can include one. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@429 PS3, Line 429: > Should we include the event too, so we can more easily debug what happened? Done http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@432 PS3, Line 432: : : : : : : : : : : : > This is kind of duplicated below; can you decompose it into a helper method I've condensed the handling a bit, but didn't create a helper method because I think it would be about a wash in terms of LOC changes. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@492 PS3, Line 492: : : : : > Holdover from before your checks below? Ack http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/sys_catalog.h File src/kudu/master/sys_catalog.h: http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/sys_catalog.h@121 PS3, Line 121: TSK_ENTRY = 4, // Token Signing Key entry. > Would be good to explain somewhere (maybe in the class comment) what the da Done http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/sys_catalog.h@170 PS3, Line 170: // Get the latest processed HMS notification log event ID. > Nit: could you work the word 'latest' or 'newest' into the name, since it's Done http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/sys_catalog.cc@647 PS3, Line 647: TRACE_EVENT0("master", "SysCatalogTable::VisitTskEntries"); > There should only be one of these entries, right? So we could DCHECK that e I've added a DCHECK that the event ID is != -1, which is what I think you meant. http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/sys_catalog.cc@810 PS3, Line 810: > How do we guarantee that, if this becomes an UPDATE, event_id is newer than We check this is the notification log listener, skipping out-of-order events. Checking here would require an extra read, so I think we should just trust. -- To view, visit http://gerrit.cloudera.org:8080/8313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775 Gerrit-Change-Number: 8313 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-Comment-Date: Tue, 10 Apr 2018 19:48:51 +0000 Gerrit-HasComments: Yes
