Jason Fehr 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 6: (9 comments) 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: CATALOG_VERSION_INCREMENT_ON_RESET (100) on every global Please explain the reason for this new functionality where min catalog version is incremented by 100 to indicate a reset. 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@144 PS6, Line 144: /// TODO: Unify this with min_catalog_version_to_serve_. Has a Jira been created for this TODO? http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.h@225 PS6, Line 225: to of Nit: remove double spaces and the word "of: 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: const int64_t CATALOG_VERSION_INCREMENT_ON_RESET = 100; It's risky having separate constants in be and fe code that must be kept in sync. If the fe code runs in a JVM on the catalog server, can this value be a hidden startup flag instead of 2 separate constants? At the very least we should have a DCHECK that ensures both constants are in sync. http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.cc@803 PS6, Line 803: if (!FLAGS_enable_catalogd_ha) { Nit: insert blank line above http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.cc@816 PS6, Line 816: lock_guard<mutex> meta_lock(ha_transition_lock_); Nit: insert blank line above http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.cc@819 PS6, Line 819: if (!is_active_) { The definition of is_active_ states this variable is protected by catalog_lock_, but this code does not hold that lock: https://gerrit.cloudera.org/c/23194/6/be/src/catalog/catalog-server.h#241 http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/catalog-server.cc@827 PS6, Line 827: if (triggered_first_reset_) return Status::OK(); Should catalog_lock_ be held here since triggered_first_reset_ is listed as being protected by that lock? http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/workload-management-init.cc File be/src/catalog/workload-management-init.cc: http://gerrit.cloudera.org:8080/#/c/23194/6/be/src/catalog/workload-management-init.cc@516 PS6, Line 516: return is_active_; Does catalog_lock_ need to be held here? -- 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: 6 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Thu, 24 Jul 2025 16:09:40 +0000 Gerrit-HasComments: Yes
