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 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/consensus/consensus_queue.cc@1054 PS3, Line 1054: The peer is actually AHEAD of the cache That's the other (remote) peer is ahead of the cache of this (local) peer, right? http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/consensus/consensus_queue.cc@1055 PS3, Line 1055: peer->wal_catchup_possible Instead, would it make sense to fix the code that sets peer->wal_catchup_possible 'false' (or not setting it 'true')? Is it somewhere under the 'if()' clause at lines 761-770? Maybe, something isn't working as expected there because of the extra condition in the criterion, such as (allow_status_msg_for_failed_peer_ == nullptr || !*allow_status_msg_for_failed_peer_ || peer_copy.wal_catchup_possible) at line 722? 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> 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? Also, why does it try to sum all the found values? The system catalog should contain only a single tablet, no? 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 this scenario is Kudu3762Test.TestAddMaster 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. 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. 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 MonoDelta::FromSeconds(120) for the timeout? 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()? http://gerrit.cloudera.org:8080/#/c/24287/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@488 PS3, Line 488: gets get 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 the test binary if any assertion triggers after creating/starting 'churn_thread'. -- 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: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Sat, 09 May 2026 02:25:17 +0000 Gerrit-HasComments: Yes
