Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/23194 )
Change subject: IMPALA-14220 (part 2): Delay AcceptRequest until catalog is stable ...................................................................... Patch Set 7: (8 comments) Thanks for the comments! I just have time to address some comments. Will update this later. Uploaded the recent state for review. http://gerrit.cloudera.org:8080/#/c/23194/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23194/6//COMMIT_MSG@26 PS6, Line 26: reset (Invalidate Metadata) initiated by catalog-server.cc. > Please explain the reason for this new functionality where min catalog vers Replaced this hack with tracking the number of resets that have started. http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.h File be/src/catalog/catalog-server.h: http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.h@225 PS6, Line 225: catalog_ > Nit: remove double spaces and the word "of: Done http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.h@236 PS6, Line 236: /// BEGIN: Members that must be protected by catalog_lock_. : /// ----------------------------------------------------------------------------- : Needs to update this. http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.cc@382 PS6, Line 382: class CatalogServiceThriftIf : public CatalogServiceIf { > It's risky having separate constants in be and fe code that must be kept in It's indeed a hack to help requests checking if catalog reset in JVM has acquires the catalog version lock and starts. Replaced this hack with exposing the num of catalog resets in JVM through JNI as Riza suggested offline. http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.cc@803 PS6, Line 803: if (current_num_reset_starts > 0) break; > Nit: insert blank line above Done. But do we have any guidance for blank lines in our code style doc? http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.cc@816 PS6, Line 816: if (!is_active_.Load()) { > Nit: insert blank line above Done http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.cc@819 PS6, Line 819: server_address)); > The definition of is_active_ states this variable is protected by catalog_l This is changed in the first patch: https://gerrit.cloudera.org/c/23168/ We need to update the comments. http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.cc@827 PS6, Line 827: RETURN_IF_ERROR(catalog_->GetNumCatalogResetStarts(¤t_num_reset_starts)); > Should catalog_lock_ be held here since triggered_first_reset_ is listed as It's a little bit tricky here. We are kind of spliting HA related responsibility of catalog_lock_ into ha_transition_lock_ (a new lock). * Before HA failover starts, is_active_ is false. Requests are rejected earlier and won't reach this check. Note that triggered_first_reset_ is true since catalogd already starts. But this value won't be used. * When HA failover starts, UpdateActiveCatalogd() acquires the ha_transition_lock_ which blocks all requests in AcceptRequest(). UpdateActiveCatalogd() sets is_active_ to true and triggered_first_reset_ to false in the protection of ha_transition_lock_. * After UpdateActiveCatalogd() returns, it releases ha_transition_lock_ so all requests can check triggered_first_reset_ in AcceptRequest(). It will only be set to true in TriggerResetMetadata() running in a background thread. So when a request sees 'triggered_first_reset_' is true here, it won't be a stale true value and can be trusted. If 'triggered_first_reset_' is false, we can continue to do the below check. -- To view, visit http://gerrit.cloudera.org:8080/23194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I370d21319335318e441ec3c3455bac4227803900 Gerrit-Change-Number: 23194 Gerrit-PatchSet: 7 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Sat, 26 Jul 2025 03:15:47 +0000 Gerrit-HasComments: Yes
