Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18866 )
Change subject: [master] KUDU-3322 / KUDU-3319 HMS event logging ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/18866/5/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/18866/5/src/kudu/integration-tests/master_hms-itest.cc@892 PS5, Line 892: TestHMSDBErase It seems this test fails with ASAN warning. Since it's not clear what's going on, we can skip the test in case of ASAN build, adding corresponding comment and TODO which we can address later on. http://gerrit.cloudera.org:8080/#/c/18866/5/src/kudu/integration-tests/master_hms-itest.cc@914 PS5, Line 914: string str = "The event ID 2 last seen by Kudu master is greater " : "than 0 currently reported by HMS. Has the HMS database " : "been reset (backup&restore, etc.)?"; nit: since this is a constant string, maybe move this to the outer scope, adding 'const' or maybe even turning it into 'constexpr const char* const' ? http://gerrit.cloudera.org:8080/#/c/18866/5/src/kudu/integration-tests/master_hms-itest.cc@919 PS5, Line 919: ASSERT_STR_CONTAINS(line, str); Isn't this always true since it's under the 'if (line.find(str) != std::string::npos)' clause? http://gerrit.cloudera.org:8080/#/c/18866/5/src/kudu/master/hms_notification_log_listener.cc File src/kudu/master/hms_notification_log_listener.cc: http://gerrit.cloudera.org:8080/#/c/18866/5/src/kudu/master/hms_notification_log_listener.cc@290 PS5, Line 290: events.empty() Could you add a comment explaining why we are checking for that precondition only in case of events.empty() is true? http://gerrit.cloudera.org:8080/#/c/18866/5/src/kudu/master/hms_notification_log_listener.cc@291 PS5, Line 291: int64_t event_id; : RETURN_NOT_OK_PREPEND(catalog_manager_->hms_catalog()-> : GetCurrentNotificationEventId(&event_id), : "failed to retrieve latest notification log event"); : if (event_id < processed_event_id) { : LOG(ERROR) << Substitute("The event ID $0 last seen by Kudu master is greater " : "than $1 currently reported by HMS. Has the HMS database " : "been reset (backup&restore, etc.)?", : processed_event_id, event_id); : } : } Since the code under 'for()' clause doesn't do anything to 'events', maybe move this to be at line 239? -- To view, visit http://gerrit.cloudera.org:8080/18866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I242c9cedf170ed621867b905f1cb1875347d5887 Gerrit-Change-Number: 18866 Gerrit-PatchSet: 5 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 22 Sep 2022 20:21:26 +0000 Gerrit-HasComments: Yes
