Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18866 )
Change subject: [master] KUDU-3322 / KUDU-3319 HMS event logging ...................................................................... Patch Set 6: (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 go Done http://gerrit.cloudera.org:8080/#/c/18866/5/src/kudu/integration-tests/master_hms-itest.cc@914 PS5, Line 914: "than 0 currently reported by HMS. Has the HMS database " : "been reset (backup&restore, etc.)?"; : ASSERT_EVENTUALLY([&] { > nit: since this is a constant string, maybe move this to the outer scope, a Done http://gerrit.cloudera.org:8080/#/c/18866/5/src/kudu/integration-tests/master_hms-itest.cc@919 PS5, Line 919: std::ifstream in(strings::Substitut > Isn't this always true since it's under the 'if (line.find(str) != std::str Right, I guess that is a remnant of when we had part of the string in the if condition. Will get rid of it. 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: > Could you add a comment explaining why we are checking for that preconditio Done http://gerrit.cloudera.org:8080/#/c/18866/5/src/kudu/master/hms_notification_log_listener.cc@291 PS5, Line 291: // TODO(KUDU-2475): Ignoring errors could result in a client receiving an : // ack for a table rename or drop which fails. : WARN_NOT_OK(s, Substitute("Failed to handle Hive Metastore notification: $0", : EventDebugString(event))); : : // Short-circuit when leadership is lost to prevent applying notification : // events out of order. : if (l.has_term_changed()) { : return Status::ServiceUnavailable( : "lost leadership while handling Hive Metastore notification log events", s.message()); : > Since the code under 'for()' clause doesn't do anything to 'events', maybe Done -- 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: 6 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 23:28:21 +0000 Gerrit-HasComments: Yes
