Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/23804 )
Change subject: IMPALA-13863: Add metric to track number of loaded tables in catalogd ...................................................................... Patch Set 3: (6 comments) The code looks much cleaner now. http://gerrit.cloudera.org:8080/#/c/23804/3/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/23804/3/fe/src/main/java/org/apache/impala/catalog/Db.java@218 PS3, Line 218: public void addTable(Table table) { tableCache_.add(table); } New codes added for catalogd could still use this and not update the counter. Can we keep only one addTable() method and always pass the update function to it? This method is also used in ImpaladCatalog, i.e. used in legacy catalog mode coordinators. But it's OK to also track the counter there. We might need to move the counter from CatalogServiceCatalog to Catalog.java. http://gerrit.cloudera.org:8080/#/c/23804/1/tests/custom_cluster/test_automatic_invalidation.py File tests/custom_cluster/test_automatic_invalidation.py: http://gerrit.cloudera.org:8080/#/c/23804/1/tests/custom_cluster/test_automatic_invalidation.py@125 PS1, Line 125: count_after_load = self.cluster.catalogd.service.get_metric_value(metric_name) > This sleep is to allow metric propagation in multi-coordinator custom clust What do you mean by "metric propagation"? Do we check metrics of coordinators? What error do you see when not adding these sleeps? http://gerrit.cloudera.org:8080/#/c/23804/1/tests/custom_cluster/test_automatic_invalidation.py@220 PS1, Line 220: # Test 7: DROP DATABASE CASCADE with loaded table > Done not done yet, please use "0". http://gerrit.cloudera.org:8080/#/c/23804/3/tests/custom_cluster/test_automatic_invalidation.py File tests/custom_cluster/test_automatic_invalidation.py: http://gerrit.cloudera.org:8080/#/c/23804/3/tests/custom_cluster/test_automatic_invalidation.py@110 PS3, Line 110: refresh, rename, and removal.""" It'd be nice to add a test that the loaded table is dropped by processing a DROP_TABLE event, e.g. impala> select * from tbl; hive> drop table tbl; wait until the events are processed, then check the counter value. http://gerrit.cloudera.org:8080/#/c/23804/3/tests/custom_cluster/test_automatic_invalidation.py@126 PS3, Line 126: initial_count + 1 nit: using "1" to be simple http://gerrit.cloudera.org:8080/#/c/23804/3/tests/custom_cluster/test_automatic_invalidation.py@154 PS3, Line 154: functional Let's use 'unique_database' so the whole db will be cleaned up even if the test fails. Just add a parameter 'unique_database' to the test method and then we can use it directly. E.g. https://github.com/apache/impala/blob/1970cc709df637f1e416198fb9a6553b2947bffa/tests/custom_cluster/test_admission_controller.py#L663 Its implementation is at https://github.com/apache/impala/blob/1970cc709df637f1e416198fb9a6553b2947bffa/tests/conftest.py#L315 -- To view, visit http://gerrit.cloudera.org:8080/23804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5aa54f9f7507709b654df22e24592799811e8b6c Gerrit-Change-Number: 23804 Gerrit-PatchSet: 3 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Sun, 28 Dec 2025 09:13:16 +0000 Gerrit-HasComments: Yes
