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 6: (9 comments) 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: Status Init(bool is_first_run); > nit: move down after Shutdown() and add a comment? Done http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/catalog_manager.h@1542 PS5, Line 1542: > nit: move above DISALLOW_COPY_AND_ASSIGN Done 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: if (catalog_->is_joining_existing > Is it required to set catalog_status_ to fake 'OK'? Set the status to ServiceUnavailable() with the corresponding message. 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_OK(new_master->W > Here and below: this looks a bit odd -- WaitForCatalogManager() is supposed This was needed to catch the leader_status_ being in uninitialized state. The patch is now revised to not have this awkward approach. 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: > nit: Raft Done http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/master/master_runner.cc@451 PS5, Line 451: AddMasterRequestPB add_req; : *add_req.mutable_rpc_addr() = HostPortToPB(local_hp); > Is this the earliest when we can do this when joining a cluster? I'd rathe Done 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: : class Kudu3762Test : public ExternalMiniClusterTest {}; : // Verifies that a new Master can join the cluster when its initial log index (0) : // is already garbage collected on the leader. This specifically tests the fix : // for KUDU-3762, ensuring a newly joining peer isn't prematurely flagged as : // unrecoverable (wal_catchup_possible = false) while its log index is still 0 : // during initialization. : TEST_F(Kudu3762Test, AddMaster) { : SKIP_IF_SLOW_NOT_ALLOWED(); : ExternalMiniClusterOptions opts; : : opts.num_masters = 2; : opts.num_tablet_servers = 3; : // We need at least one WAL segment to be garbage collected to reproduce the issue. So : // ensure we flush to the disk aggressively while retaining lower number of smaller sized : // log segments. More number of maintenance manager threads with low polling intervals : // increase the odd of the WAL segment getting garbage collected : opts.extra_master_flags.emplace_back("--flush_threshold_mb=0"); : opts.extra_master_flags.emplace_back("--flush_threshold_secs=1"); : opts.extra_master_flags.emplace_back("--log_cache_size_limit_mb=1"); : opts.extra_master_flags.emplace_back("--log_compression_codec=no_compression"); : opts.extra_master_flags.emplace_back("--log_max_segments_to_retain=3"); : opts.extra_master_flags.emplace_back("--log_segment_size_mb=1"); : opts.extra_master_flags.emplace_back("--maintenance_manager_num_threads=4"); : opts.extra_master_flags.emplace_back("--maintenance_manager_polling_interval_ms=10"); : opts.extra_master_flags.emplace_back("--unlock_experimental_flags=true"); : : unique_ptr<ExternalMiniCluster> cluster(new ExternalMiniCluster(opts)); : ASSERT_OK(cluster->Start()); : : // Create a client to add and delete a table in loop to constantly add new entries/indices : // in the WAL of system catalog : > Could GetInt64Metric() from cluster_test_util fit the bill instead of this Done http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/mini-cluster/external_mini_cluster-test.cc@426 PS5, Line 426: ld(&schema)); > Does it do so indeed? I was under an impression that the tablet copy only Yes that is true, let me clarify the comment. http://gerrit.cloudera.org:8080/#/c/24287/5/src/kudu/mini-cluster/external_mini_cluster-test.cc@519 PS5, Line 519: : : : > nit: remove this, but update SCOPED_CLEANUP to a named variable of MakeScop 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: 6 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 22:22:14 +0000 Gerrit-HasComments: Yes
