Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24166 )
Change subject: IMPALA-14684: Add metrics for catalogd table invalidations ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/24166/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24166/7//COMMIT_MSG@17 PS7, Line 17: 01s Double checked this, I realized metrics are captured by catalog-metrics-refresh-thread which sleeps 1s in each iteration: https://github.com/apache/impala/blob/b38852802b493b69159b1c0230e992f3e7237b28/be/src/catalog/catalog-server.cc#L1120 So it's possible that this 1s update is ignored and not captured. How about using a longer period like 10s? http://gerrit.cloudera.org:8080/#/c/24166/5/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/24166/5/common/thrift/metrics.json@670 PS5, Line 670: TIME > Done Sorry to be back and forth here. I found the value is rendered as something like "495721h12m". Then I realized this is for duration. We don't have a unit for timestamps so probably we should still use "NONE". http://gerrit.cloudera.org:8080/#/c/24166/7/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java File fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java: http://gerrit.cloudera.org:8080/#/c/24166/7/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@328 PS7, Line 328: synchronized (lock) { I don't think we need this additional lock since invalidateSome() and invalidateOlderThan() are invoked inside the block of synchronized (CatalogdTableInvalidator.this): https://github.com/apache/impala/blob/b38852802b493b69159b1c0230e992f3e7237b28/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java#L273 http://gerrit.cloudera.org:8080/#/c/24166/7/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@338 PS7, Line 338: private void cleanupOldBatches(ArrayDeque<InvalidationBatch> batches) { This method has only one usage in recordInvalidationBatch(). It'd be better to merge it there. http://gerrit.cloudera.org:8080/#/c/24166/7/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@351 PS7, Line 351: synchronized (lock) { We can declare the method with "synchronized" to use the same object lock as the writer: https://github.com/apache/impala/blob/b38852802b493b69159b1c0230e992f3e7237b28/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java#L273 http://gerrit.cloudera.org:8080/#/c/24166/7/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@354 PS7, Line 354: for (InvalidationBatch batch : batches) { : if (batch.timestampNanos_ >= cutoffTime) { : count += batch.count_; : } : } Small optimization: start from the last batch and count forward till the one that have timestamp < cutoffTime. http://gerrit.cloudera.org:8080/#/c/24166/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/24166/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java@661 PS7, Line 661: response.setCatalog_ttl_invalidations_1s( : invalidator.getTtlInvalidations1SecCount()); : response.setCatalog_ttl_invalidations_1m( : invalidator.getTtlInvalidations1MinCount()); : response.setCatalog_ttl_invalidations_5m( : invalidator.getTtlInvalidations5MinCount()); : response.setCatalog_ttl_invalidations_30m( : invalidator.getTtlInvalidations30MinCount()); : response.setCatalog_memory_pressure_invalidations_1s( : invalidator.getMemoryPressureInvalidations1SecCount()); : response.setCatalog_memory_pressure_invalidations_1m( : invalidator.getMemoryPressureInvalidations1MinCount()); : response.setCatalog_memory_pressure_invalidations_5m( : invalidator.getMemoryPressureInvalidations5MinCount()); : response.setCatalog_memory_pressure_invalidations_30m( : invalidator.getMemoryPressureInvalidations30MinCount()); : response.setCatalog_last_ttl_invalidation_ms( : invalidator.getLastTtlInvalidationMillis()); : response.setCatalog_last_ttl_invalidated_tables( : invalidator.getLastTtlInvalidatedTables()); : response.setCatalog_last_memory_pressure_invalidation_ms( : invalidator.getLastMemoryPressureInvalidationMillis()); : response.setCatalog_last_memory_pressure_invalidated_tables( : invalidator.getLastMemoryPressureInvalidatedTables()); These all acquires the lock individually, which is a waste and could see inconsistent results. Can we get the metrics in one invocation so just need to acquire the lock once? E.g. using a method like invalidator.getMetrics(). http://gerrit.cloudera.org:8080/#/c/24166/7/tests/custom_cluster/test_automatic_invalidation.py File tests/custom_cluster/test_automatic_invalidation.py: http://gerrit.cloudera.org:8080/#/c/24166/7/tests/custom_cluster/test_automatic_invalidation.py@47 PS7, Line 47: metrics_test_ttl_s = 60 The test takes 150s. Can we use a shorter TTL, e.g. 1s? -- To view, visit http://gerrit.cloudera.org:8080/24166 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If78e6c8429801e3c1829860666f04ad287b826ab Gerrit-Change-Number: 24166 Gerrit-PatchSet: 7 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 21 Jul 2026 02:13:57 +0000 Gerrit-HasComments: Yes
