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 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/18866/4/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/18866/4/src/kudu/integration-tests/master_hms-itest.cc@886 PS4, Line 886: ExternalMiniClusterITestBase::SetUp(); : : ExternalMiniClusterOptions opts; : opts.hms_mode = GetHmsMode(); : opts.num_masters = 1; : opts.num_tablet_servers = 1; : opts.enable_kerberos = EnableKerberos(); : opts.principal = Principal(); : // Tune down the notification log poll period in order to speed up catalog convergence. : opts.extra_master_flags.emplace_back("--hive_metastore_notification_log_poll_period_seconds=1"); : opts.extra_master_flags.emplace_back("--logtostderr=false"); : StartClusterWithOpts(std::move(opts)); : : thrift::ClientOptions hms_opts; : hms_opts.enable_kerberos = EnableKerberos(); : hms_opts.service_principal = "hive"; : ASSERT_OK(harness_.RestartHmsClient(cluster_, hms_opts)); Alternatively, instead of copying over the implementation of several helper methods from MasterHmsTest, I'd think of introducing a virtual method named 'PrepareCluster()' or something like that into MasterHmsTest to put there all this stuff, and call that method in the SetUp() method, while deriving MasterHmsDBTest from MasterHmsTest. Now, the newly added MasterHmsDBTest needs to override just PrepareCluster(), and there is no need to copy over the implementation of helper methods. A better approach would be introducing a virtual method into MasterHmsTest to fill in the instance of ExternalMiniClusterOptions used in SetUp(), or just a method that produces the list of master flags. The the derived MasterHmsDBTest needs to override only that method. http://gerrit.cloudera.org:8080/#/c/18866/4/src/kudu/integration-tests/master_hms-itest.cc@952 PS4, Line 952: With just 1 event created With just one event generated, ... http://gerrit.cloudera.org:8080/#/c/18866/4/src/kudu/integration-tests/master_hms-itest.cc@953 PS4, Line 953: and hence creating more : // than one event. nit: this part might be dropped? http://gerrit.cloudera.org:8080/#/c/18866/4/src/kudu/integration-tests/master_hms-itest.cc@969 PS4, Line 969: string line; : bool log_found = false; nit: why not to move these into the ASSERT_EVENTUALLY scope below since they are not used anywhere else? http://gerrit.cloudera.org:8080/#/c/18866/4/src/kudu/master/hms_notification_log_listener.cc File src/kudu/master/hms_notification_log_listener.cc: http://gerrit.cloudera.org:8080/#/c/18866/4/src/kudu/master/hms_notification_log_listener.cc@296 PS4, Line 296: Found the last seen event ID in the local Kudu master($0) " : "to be greater than the latest event ID in HMS($1) How about: The event ID $0 last seen by Kudu master is greater than $1 currently reported by HMS. http://gerrit.cloudera.org:8080/#/c/18866/4/src/kudu/master/hms_notification_log_listener.cc@298 PS4, Line 298: Was there any backup or restore done on HMS recently? How about: Has the HMS database been reset (backup&restore, etc.)? -- 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: 4 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: Wed, 14 Sep 2022 22:37:24 +0000 Gerrit-HasComments: Yes
