Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22640 )
Change subject: IMPALA-13850 (part 2): Implement in-place reset for CatalogD ...................................................................... Patch Set 8: (7 comments) http://gerrit.cloudera.org:8080/#/c/22640/8/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/22640/8/fe/src/main/java/org/apache/impala/catalog/Catalog.java@90 PS8, Line 90: Uses an AtomicReference so reset() : // operations can atomically swap dbCache_ references. nit: stale comment http://gerrit.cloudera.org:8080/#/c/22640/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/22640/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1402 PS8, Line 1402: (db1, db2) -> db1.getName().compareTo(db2.getName()) nit: can be simplified to dbList_.sort(Comparator.comparing(Db::getName)); http://gerrit.cloudera.org:8080/#/c/22640/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1420 PS8, Line 1420: waitOngoingResetMetadata(nextDb.getName()); If reset() runs long, getCatalogDelta() still waits that long and coordinators in legacy catalog mode still can't get the initial catalog update. Maybe we can add a new state of IncompleteDb and send that for unloaded dbs that are still in resettingDbs_. Coordinator in legacy catalog mode waits until an IncompleteDb becomes a loaded Db before using it. http://gerrit.cloudera.org:8080/#/c/22640/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2466 PS8, Line 2466: resetMetadataCondition_.signalAll(); The current thread is still holding the write lock at this point. Will this really wake up other waiting threads? Should we use resetMetadataCondition_.await()? http://gerrit.cloudera.org:8080/#/c/22640/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@4400 PS8, Line 4400: resp.catalog_info.db_names = ImmutableList.copyOf(dbCache_.keySet()); Some dbs might be missing here if reset() runs long and hasn't added them into dbCache_. I think queries on these dbs will fail. Should we add names in resettingDbs_? http://gerrit.cloudera.org:8080/#/c/22640/8/fe/src/main/java/org/apache/impala/util/DebugUtils.java File fe/src/main/java/org/apache/impala/util/DebugUtils.java: http://gerrit.cloudera.org:8080/#/c/22640/8/fe/src/main/java/org/apache/impala/util/DebugUtils.java@111 PS8, Line 111: lable nit: "label" http://gerrit.cloudera.org:8080/#/c/22640/8/tests/custom_cluster/test_catalogd_ha.py File tests/custom_cluster/test_catalogd_ha.py: http://gerrit.cloudera.org:8080/#/c/22640/8/tests/custom_cluster/test_catalogd_ha.py@285 PS8, Line 285: @UniqueDatabase.parametrize(name_prefix='aa_test_catalogd_auto_failover_slow') Can we add another test using a db name that is the last one to be loaded? -- To view, visit http://gerrit.cloudera.org:8080/22640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4ae2154612746b34484391c5950e74b61f85c9d Gerrit-Change-Number: 22640 Gerrit-PatchSet: 8 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <k.venureddy2...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Tue, 01 Apr 2025 01:46:45 +0000 Gerrit-HasComments: Yes
