You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Quanlong Huang (Code Review)" <ge...@cloudera.org> on 2026/05/06 06:52:49 UTC
[Impala-ASF-CR] IMPALA-14684: Add metrics for catalogd table invalidations
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 2:
(18 comments)
Thanks for adding these metrics!
http://gerrit.cloudera.org:8080/#/c/24166/2/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/24166/2/be/src/catalog/catalog-server.cc@1191
PS2, Line 1191: // IMPALA-14684: Update table invalidation metrics
nit: remove trivial comments
http://gerrit.cloudera.org:8080/#/c/24166/2/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/24166/2/common/thrift/JniCatalog.thrift@1126
PS2, Line 1126: IMPALA-14684:
nit: don't need to mention the JIRA id here. It's just a new feature.
http://gerrit.cloudera.org:8080/#/c/24166/2/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/24166/2/fe/src/main/java/org/apache/impala/catalog/Catalog.java@131
PS2, Line 131: // IMPALA-14684: Track invalidations to help monitor automatic table invalidation.
nit: we don't need to list the JIRA ids for each change unless it's a critical regression and the JIRA has more details. Please remove this and the other two on L126 and L135.
http://gerrit.cloudera.org:8080/#/c/24166/2/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/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@104
PS2, Line 104: IMPALA-14684:
nit: please remove these JIRA ids.
http://gerrit.cloudera.org:8080/#/c/24166/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@105
PS2, Line 105: * Timestamps older than 30 minutes are periodically cleaned up.
nit: Please add a comment about why we use CopyOnWriteArrayList.
http://gerrit.cloudera.org:8080/#/c/24166/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@242
PS2, Line 242: long timestamp = TIME_SOURCE.read();
It'd be helpful to expose this timestamp and 'numInvalidated' as metrics so admin knows when did this happen and its impact:
* last-memory-pressure-invalidation-ns
* last-memory-pressure-invalidated-tables
* last-ttl-invalidation-ns
* last-ttl-invalidated-tables
http://gerrit.cloudera.org:8080/#/c/24166/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@245
PS2, Line 245: memoryPressureInvalidationTimestamps_.add(timestamp);
Use addAll() on Collections.nCopies(numInvalidated, Long.valueOf(timestamp)) to avoid modifying (and copying) the array multiple times.
incrementMemoryPressureInvalidatedTables() also need an override to increase N at once.
The TTL part has the same issue.
http://gerrit.cloudera.org:8080/#/c/24166/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@308
PS2, Line 308: index = -(index + 1);
What does this mean? Does it assume index is -1?
http://gerrit.cloudera.org:8080/#/c/24166/2/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/2/fe/src/main/java/org/apache/impala/service/JniCatalog.java@660
PS2, Line 660: // IMPALA-14684: Add table invalidation metrics
nit: remove trivial comments
http://gerrit.cloudera.org:8080/#/c/24166/2/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
File fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java:
http://gerrit.cloudera.org:8080/#/c/24166/2/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@87
PS2, Line 87: * IMPALA-14684: Test table invalidation metrics.
nit: remove commits like this. Don't need to mention the JIRA.
http://gerrit.cloudera.org:8080/#/c/24166/2/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@142
PS2, Line 142: Assert.assertTrue("1-sec count should be non-negative", count1s >= 0);
: Assert.assertTrue("1-min count should be non-negative", count1m >= 0);
: Assert.assertTrue("5-min count should be non-negative", count5m >= 0);
: Assert.assertTrue("30-min count should be non-negative", count30m >= 0);
Remove these since the asserts on L148-151 already cover them.
http://gerrit.cloudera.org:8080/#/c/24166/2/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@148
PS2, Line 148: Assert.assertEquals("1-sec window should show 2 invalidations", 2, count1s);
: Assert.assertEquals("1-min window should show 2 invalidations", 2, count1m);
: Assert.assertEquals("5-min window should show 2 invalidations", 2, count5m);
: Assert.assertEquals("30-min window should show 2 invalidations", 2, count30m);
These metrics are in the same value. We need this test to verify different sliding windows. Probably by using a longer TTL, e.g. 30m and load the tables (might need more tables) at different time.
http://gerrit.cloudera.org:8080/#/c/24166/2/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@194
PS2, Line 194: initialMemoryCount, catalog_.getNumMemoryPressureInvalidatedTables());
This doesn't worth a new test. We can just verify it in the above testTtlInvalidationMetrics (rename it to testInvalidationMetrics).
http://gerrit.cloudera.org:8080/#/c/24166/2/tests/custom_cluster/test_automatic_invalidation.py
File tests/custom_cluster/test_automatic_invalidation.py:
http://gerrit.cloudera.org:8080/#/c/24166/2/tests/custom_cluster/test_automatic_invalidation.py@220
PS2, Line 220: Test IMPALA-14684:
nit: remove the JIRA id.
http://gerrit.cloudera.org:8080/#/c/24166/2/tests/custom_cluster/test_automatic_invalidation.py@269
PS2, Line 269: self.execute_query("select count(*) from %s" % table2)
: self.execute_query("select count(*) from %s" % table3)
We can use an EXPLAIN query that joins multiple tables to make more tables loaded.
http://gerrit.cloudera.org:8080/#/c/24166/2/tests/custom_cluster/test_automatic_invalidation.py@301
PS2, Line 301: assert ttl_count_1m >= 0, (
: "TTL 1-min count should be non-negative (got %d)" % ttl_count_1m)
: assert ttl_count_5m >= 0, (
: "TTL 5-min count should be non-negative (got %d)" % ttl_count_5m)
: assert ttl_count_30m >= 0, (
: "TTL 30-min count should be non-negative (got %d)" % ttl_count_30m)
: assert memory_count_1s >= 0, (
: "Memory pressure 1-sec count should be non-negative (got %d)" % memory_count_1s)
: assert memory_count_1m >= 0, (
: "Memory pressure 1-min count should be non-negative (got %d)" % memory_count_1m)
Remove these since L314-319 already assures them.
http://gerrit.cloudera.org:8080/#/c/24166/2/tests/custom_cluster/test_automatic_invalidation.py@347
PS2, Line 347: catalogd.get_metric_value(memory_metric)
not used?
http://gerrit.cloudera.org:8080/#/c/24166/2/tests/custom_cluster/test_automatic_invalidation.py@370
PS2, Line 370: % memory_count_1m)
Can we also check other metrics, e.g. for 5m, 30m?
--
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: 2
Gerrit-Owner: Arnab Karmakar <ar...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 06 May 2026 06:52:49 +0000
Gerrit-HasComments: Yes