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 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/24287/7/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/24287/7/src/kudu/master/catalog_manager.h@1544 PS7, Line 1544: nit: remove the extra string? http://gerrit.cloudera.org:8080/#/c/24287/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/24287/7/src/kudu/master/catalog_manager.cc@800 PS7, Line 800: && !catalog_manager_->is_joining_existing_cluster_ It might be a bit cleaner if moving this condition inside the loop: while (!NoBarrier_Load(&closing_)) { if (PREDICT_FALSE(catalog_manager_->is_joining_existing_cluster_)) { break; } ... } http://gerrit.cloudera.org:8080/#/c/24287/7/src/kudu/master/catalog_manager.cc@1011 PS7, Line 1011: is_joining_existing_cluster_ = joining_existing_cluster; I think it's prudent to check for catalog_status_ here before setting is_joining_existing_cluster_. Otherwise, due to the hack in ScopedLeaderSharedLock::CheckIsInitializedOrRespond(), there might be an unexpected behavior. http://gerrit.cloudera.org:8080/#/c/24287/7/src/kudu/master/catalog_manager.cc@7316 PS7, Line 7316: catalog_->is_joining_existing_cluster_ nit: does it make sense to wrap this into PREDICT_FALSE? The ScopedLeaderSharedLock constructor is called very often in the catalog manager's code and around, and adding a master is an extremely rare case http://gerrit.cloudera.org:8080/#/c/24287/7/src/kudu/master/catalog_manager.cc@7317 PS7, Line 7317: catalog_status_ = Status::ServiceUnavailable("System Catalog Consensus is shutdown" nit: add a space between two joined parts of the string http://gerrit.cloudera.org:8080/#/c/24287/7/src/kudu/master/catalog_manager.cc@7317 PS7, Line 7317: Consensus Is that the same as 'Raft Consensus'? Maybe, then introduce a static constant Status and use it in both cases, assigning to catalog_status_ and leader_status_? http://gerrit.cloudera.org:8080/#/c/24287/7/src/kudu/master/catalog_manager.cc@7319 PS7, Line 7319: leader_status_ = Status::ServiceUnavailable("Raft Consensus has been stopped as the" ditto http://gerrit.cloudera.org:8080/#/c/24287/7/src/kudu/master/catalog_manager.cc@7359 PS7, Line 7359: && !catalog_->is_joining_existing_cluster_ This doesn't smell particularly good. Isn't there a risk of hitting undefined behavior at call sites of CheckIsInitializedOrRespond() due to this when adding a new master? How do we tell between two possibilities (1) the system catalog was successfully initialized and then we set is_joining_existing_cluster_ = true and (2) the system catalog hasn't initialized yet, but we set is_joining_existing_cluster_ = true? If adding this, it's prudent to check for catalog_status_ in CatalogManager::SetJoiningCluster() -- I added a comment there. Is it really necessary to add this hack here? Wouldn't it be safer to add a shortcut with 'if (is_joining_existing_cluster_)' clause in a few call sites where CheckIsInitializedOrRespond() now responds with an error if not adding this? Regardless of the former two items, it might be a bit cleaner and easier for the compiler to optimize if moving this condition under the top-level 'if ()' clause: if (PREDICT_FALSE(!catalog_status_.ok())) { if (PREDICT_FALSE(catalog_->is_joining_existing_cluster_)) { return true; } ... } -- 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: 7 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 23:55:21 +0000 Gerrit-HasComments: Yes
