Arnab Karmakar 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 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/23804/1/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/23804/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2823 PS1, Line 2823: public @Nullable Table addIncompleteTable( : String dbName, String tblName, TImpalaTableType tblType, String tblComment) { : return addIncompleteTable(dbName, tblName, tblType, tblComment, -1L); : } : : /** : * Adds a table with the given name to the catalog and returns the new table. : */ : public @Nullable Table addIncompleteTable(String dbName, String tblName, : TImpalaTableType tblType, String tblComment, long createEventId) { : try (WriteLockAndLookupDb result = new WriteLockAndLookupDb(dbName)) { : // IMPALA-9211: get db object after holding the writeLock in case of getting stale : // db object due to concurrent INVALIDATE METADATA : Db db = result.getDb(); : if (db == null) return null; : Table existingTbl = db.getTable(tblName); : if (existingTbl instanceof HdfsTable) { : // Add the old instance to the deleteLog_ so we can send isDeleted updates for : // its partitions. : existingTbl.setCatalogVersion(incrementAndGetCatalogVersion()); : deleteLog_.addRemovedObject(existingTbl.toMinimalTCatalogObject()); : } > I see we are adding codes like this before all the call sites of Db#addTabl Thanks for the tip! Moved the logic into CatalogObjectCache where it's automatically invoked for all table additions and removals. http://gerrit.cloudera.org:8080/#/c/23804/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/23804/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@854 PS1, Line 854: removeTable > I think we should update the counter here if tblToBeRemoved is a loaded tab Done 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@105 PS1, Line 105: @pytest.mark.execute_serially : @CustomClusterTestSuite.with_args(catalogd_args=timeout_flag, impalad_args=timeout_flag) : def test_loaded_tables_metric(self, cursor): : """Test IMPALA-13863: catalog.num-loaded-tables metric tracks loaded tables : correctly across various metadata operations including loading, invalidation, : refresh, rename, and removal.""" : metric_name = "catalog.num-loaded-tables" : : # Test 1: Loading increases counter : cursor.execute("invalidate metadata") : # Brief sle > Can we use the existing get_metric_value() method? https://github.com/apach Done http://gerrit.cloudera.org:8080/#/c/23804/1/tests/custom_cluster/test_automatic_invalidation.py@122 PS1, Line 122: cursor.execute(self.query) > There are some scenarios that worth testing: Done 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) > Why do we need this sleep? This sleep is to allow metric propagation in multi-coordinator custom cluster http://gerrit.cloudera.org:8080/#/c/23804/1/tests/custom_cluster/test_automatic_invalidation.py@156 PS1, Line 156: cursor.execute("create table functional.test_rename_tbl (id int)") > Shouldn't we expect count_after_global_invalidate == 0 and count_with_table Done http://gerrit.cloudera.org:8080/#/c/23804/1/tests/custom_cluster/test_automatic_invalidation.py@164 PS1, Line 164: time.sleep(2) > Can we assert count_after_create == 0? Done http://gerrit.cloudera.org:8080/#/c/23804/1/tests/custom_cluster/test_automatic_invalidation.py@207 PS1, Line 207: time.sleep(2) > Can we assert count_before_timeout == 1, or what other tables could be load Done 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 > Can we assert count_after_timeout == 0? Done -- 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: 2 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 07:02:31 +0000 Gerrit-HasComments: Yes
