Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/24287 )
Change subject: KUDU-3762 - Fix catchup deadlock on LMP mismatch ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: PS3: As discussed offline with Alexey, changed the logic of the fix to preventative by shutting down the consensus of the new master's system catalog just before it sends the AddMaster(). http://gerrit.cloudera.org:8080/#/c/24287/3/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/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@20 PS3, Line 20: #include <stdint.h> > replace with C++ counterpart: <cstdint> Done http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@416 PS3, Line 416: found = true; > Shouldn't there be 'break' after line? Done http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@424 PS3, Line 424: TEST_F(Kudu3762Test, TestAddMaster) { > Please add a concise summary of what this test scenario is doing. Done http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@424 PS3, Line 424: TestAddMaster > Consider renamin TestAddMaster -- AddMaster; as of PS3 the full name of thi Done http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@428 PS3, Line 428: opts.num_masters = 2; > It would be nice to document the 'why' part of these custom settings. Done http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@463 PS3, Line 463: ASSERT_OK(table_creator->table_name(table_name) : .schema(&schema) : .num_replicas(3) : .add_hash_partitions({"key"}, 16) : .Create()); : ASSERT_EVENTUALLY([&] { : bool in_progress = true; : ASSERT_OK(new_client->IsCreateTableInProgress(table_name, &in_progress)); : ASSERT_FALSE(in_progress); : }); : ASSERT_OK(new_client->DeleteTable(table_name)) > If any of these fail during an iteration, I believe churn thread will conti Thanks for the insight. Refactored the churn thread to propagate Status errors to the main test thread, preventing silent failures. http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@479 PS3, Line 479: while (true) { > Instead of this custom while(true) loop, maybe use AssertEventually() with Done http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@480 PS3, Line 480: CHECK_OK > Why not ASSERT_OK()? Done http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@488 PS3, Line 488: gets > get I think it should be 'gets', right? http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@495 PS3, Line 495: churn_thread.join(); > Consider moving this into a scoped cleanup to avoid undefined behavior of t Done -- 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: 3 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 00:32:42 +0000 Gerrit-HasComments: Yes
