Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24637 )
Change subject: KUDU-3751: master exits cleanly on catch-up timeout ...................................................................... Patch Set 13: (8 comments) http://gerrit.cloudera.org:8080/#/c/24637/13/src/kudu/master/catalog_manager-test.cc File src/kudu/master/catalog_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/24637/13/src/kudu/master/catalog_manager-test.cc@416 PS13, Line 416: // A LogSink that safely allows a waiter to check, from another thread, : // whether a message matching a given substring has been logged. Unlike : // StringVectorSink (see logging_test_util.h), this is safe to query : // concurrently while logging is still happening on a background thread: : // both send() and HasMessageContaining() take the same mutex. : class WaitableLogSink : public google::LogSink { : public: : void send(google::LogSeverity /*severity*/, const char* /*full_filename*/, : const char* /*base_filename*/, int /*line*/, : const google::LogMessageTime& /*tm_time*/, : const char* message, size_t message_len) override { : std::lock_guard<std::mutex> l(mutex_); : messages_.emplace_back(message, message_len); : } : : bool HasMessageContaining(const string& substr) { : std::lock_guard<std::mutex> l(mutex_); : for (const auto& msg : messages_) { : if (msg.find(substr) != string::npos) { : return true; : } : } : return false; : } : : private: : std::mutex mutex_; : std::vector<string> messages_; : }; Where is this used as of PS13? http://gerrit.cloudera.org:8080/#/c/24637/13/src/kudu/master/catalog_manager-test.cc@475 PS13, Line 475: 3 nit: introduce a constant for this, and use the constant here and elsewhere instead of hard-coded 3. http://gerrit.cloudera.org:8080/#/c/24637/13/src/kudu/master/catalog_manager-test.cc@479 PS13, Line 479: } nit: wrong ident http://gerrit.cloudera.org:8080/#/c/24637/13/src/kudu/master/catalog_manager-test.cc@532 PS13, Line 532: // A LogSink helper for checking a master's pending write op count is : // not needed here; instead, poll the sys catalog tablet's own : // /metrics endpoint directly. : auto has_pending_write_op = [&](int idx) { : EasyCurl c; : faststring buf; : Status s = c.FetchURL( : Substitute("http://$0/metrics?ids=$1";, : cluster_->master(idx)->bound_http_hostport().ToString(), : SysCatalogTable::kSysCatalogTabletId), : &buf); : if (!s.ok()) return false; : : JsonReader reader(buf.ToString()); : if (!reader.Init().ok()) return false; : vector<const rapidjson::Value*> entities; : if (!reader.ExtractObjectArray(reader.root(), nullptr, &entities).ok() || : entities.size() != 1) { : return false; : } : vector<const rapidjson::Value*> metrics; : if (!reader.ExtractObjectArray(entities[0], "metrics", &metrics).ok()) { : return false; : } : for (const auto* m : metrics) { : string name; : if (!reader.ExtractString(m, "name", &name).ok()) continue; : if (name == "write_transactions_inflight") { : int64_t value; : if (reader.ExtractInt64(m, "value", &value).ok()) { : return value > 0; : } : } : } : return false; : }; Why not to use itest::GetInt64Metric() instead of this? http://gerrit.cloudera.org:8080/#/c/24637/13/src/kudu/master/catalog_manager-test.cc@587 PS13, Line 587: ASSERT_TRUE(set_flag_status.ok()) nit for here and elsewhere: consider using ASSERT_OK() instead http://gerrit.cloudera.org:8080/#/c/24637/13/src/kudu/master/catalog_manager-test.cc@645 PS13, Line 645: 15000) Where does this extra come from? Also, if it's so long, this test scenarios must include SKIP_IF_SLOW_NOT_ALLOWED() http://gerrit.cloudera.org:8080/#/c/24637/13/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/24637/13/src/kudu/master/catalog_manager.cc@1491 PS13, Line 1491: std::_Exit(1); We have been using _exit() elsewhere in the code (e.g., subprocess.cc, fault_injection.cc, etc.). Why not to use it here as well? http://gerrit.cloudera.org:8080/#/c/24637/13/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/24637/13/src/kudu/mini-cluster/external_mini_cluster.cc@1599 PS13, Line 1599: nit: wrong indent? -- To view, visit http://gerrit.cloudera.org:8080/24637 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c9c5a3a6ec43c5174f8d8e92e2421029c51ea01 Gerrit-Change-Number: 24637 Gerrit-PatchSet: 13 Gerrit-Owner: Jeison Romero <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Jeison Romero <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Mon, 24 Aug 2026 20:40:22 +0000 Gerrit-HasComments: Yes
