Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24287 )
Change subject: KUDU-3762 - Fix catchup deadlock on LMP mismatch ...................................................................... Patch Set 5: (9 comments) Overall looks OK. A few questions and nits. Thank you for fixing this! http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/catalog_manager.h@650 PS5, Line 650: void SetJoiningCluster(bool joining_existing_cluster); nit: move down after Shutdown() and add a comment? http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/catalog_manager.h@1542 PS5, Line 1542: bool is_joining_existing_cluster_ = false; nit: move above DISALLOW_COPY_AND_ASSIGN nit: initialize along with other fields in the constructor's initializers list http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/catalog_manager.cc@7316 PS5, Line 7316: catalog_status_ = Status::OK(); Is it required to set catalog_status_ to fake 'OK'? Please at add a comment to explain why. I'd rather expect it to be set to something that meant 'not available' or 'not supported', etc. http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/dynamic_multi_master-test.cc@1679 PS5, Line 1679: ASSERT_EVENTUALLY([&] { Here and below: this looks a bit odd -- WaitForCatalogManager() is supposed to do the waiting, and its current timeout is kMasterCatalogManagerTimeoutSeconds, which is 60 seconds. Are you sure ASSERT_EVENTUALLY() with default timeout of 30 seconds really helpful around WaitForCatalogManager()? http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/master_runner.cc File src/kudu/master/master_runner.cc: http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/master_runner.cc@450 PS5, Line 450: raft nit: Raft http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/master_runner.cc@451 PS5, Line 451: server->catalog_manager()->SetJoiningCluster(true); : server->catalog_manager()->sys_catalog()->tablet_replica()->Shutdown(); Is this the earliest when we can do this when joining a cluster? I'd rather expect this to happen right after LOG(INFO) at line 429. BTW, does it make sense to add a comment to refer to KUDU-3762, explaining why it's necessary to shutdown Raft before shutting down the whole server at line 466 below? http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/mini-cluster/external_mini_cluster-test.cc File src/kudu/mini-cluster/external_mini_cluster-test.cc: http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/mini-cluster/external_mini_cluster-test.cc@389 PS5, Line 389: Status GetMasterWalGcCount(ExternalMiniCluster* cluster, int64_t* count) { : *count = 0; : auto* leader = cluster->leader_master(); : if (!leader) { : return Status::ServiceUnavailable("Leader master not found"); : } : string url = Substitute("http://$0/jsonmetricz?metrics=log_gc_duration";, : leader->bound_http_hostport().ToString()); : EasyCurl curl; : faststring dst; : RETURN_NOT_OK(curl.FetchURL(url, &dst)); : JsonReader r(dst.ToString()); : RETURN_NOT_OK(r.Init()); : vector<const Value*> entities; : RETURN_NOT_OK(r.ExtractObjectArray(r.root(), nullptr, &entities)); : for (const Value* entity : entities) { : string type; : RETURN_NOT_OK(r.ExtractString(entity, "type", &type)); : if (type == "tablet") { : vector<const Value*> metrics; : RETURN_NOT_OK(r.ExtractObjectArray(entity, "metrics", &metrics)); : for (const Value* metric : metrics) { : string name; : RETURN_NOT_OK(r.ExtractString(metric, "name", &name)); : if (name == "log_gc_duration") { : RETURN_NOT_OK(r.ExtractInt64(metric, "total_count", count)); : return Status::OK(); // Exit immediately once found. : } : } : } : } : return Status::NotFound("log_gc_duration metric not found on leader"); : } Could GetInt64Metric() from cluster_test_util fit the bill instead of this blurb? http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/mini-cluster/external_mini_cluster-test.cc@426 PS5, Line 426: once the peer catches up via tablet copy Does it do so indeed? I was under an impression that the tablet copy only helps to start with a not-yet-GCed segment, but the actual catch-up is performed via the Raft protocol, no? http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/mini-cluster/external_mini_cluster-test.cc@519 PS5, Line 519: stop_churn = true; : if (churn_thread.joinable()) { : churn_thread.join(); : } nit: remove this, but update SCOPED_CLEANUP to a named variable of MakeScopedCleanup type, and call MakeScopedCleanup::run() here instead. -- To view, visit http://gerrit.cloudera.org:8080/24287 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00f4f840c21cc037a2aa6c023af168352fef0761 Gerrit-Change-Number: 24287 Gerrit-PatchSet: 5 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Fri, 15 May 2026 05:09:04 +0000 Gerrit-HasComments: Yes
