[Impala-ASF-CR] IMPALA-14684: Add metrics for catalogd table invalidations

["Quanlong Huang (Code Review)" <ge...@cloudera.org>]

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 5:

(5 comments)

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: NONE
Let's use TIME_MS here (after changing the metric to be in milliseconds).


http://gerrit.cloudera.org:8080/#/c/24166/5/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/5/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@107
PS5, Line 107:    * CopyOnWriteArrayList is used so the list stays randomly accessible,
             :    * and Collections.binarySearch can find window boundaries in O(log n).
ArrayList is also randomly accessible and supports binary search.


http://gerrit.cloudera.org:8080/#/c/24166/5/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@255
PS5, Line 255:       long timestamp = TIME_SOURCE.read();
TIME_SOURCE.read() returns a time value representing the number of nanoseconds elapsed since some fixed but arbitrary point in time. It can only be used to measure elapsed time. Let's use System.currentTimeMillis() instead and change the metric names correspondingly.


http://gerrit.cloudera.org:8080/#/c/24166/5/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@260
PS5, Line 260: Collections.nCopies(numInvalidated, Long.valueOf(timestamp))
These N items are added together and evicted together, which seems a waste. Can we track the pairs of (timestamp, numInvalidated) instead? This makes the list much shorter.


http://gerrit.cloudera.org:8080/#/c/24166/5/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@345
PS5, Line 345:     int index = Collections.binarySearch(timestamps, cutoffTime);
CopyOnWriteArrayList can't protect this from concurrent cleanup that shrinks the array in the daemon thread.
Collections.binarySearch() invokes Collections.indexedBinarySearch() which capture the array length at start and keep using list.get() to check items. If the array changes during its execution, it might hit IndexOutOfBoundsException.

I think we should use something like ArrayDeque with a lock.



-- 
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: 5
Gerrit-Owner: Arnab Karmakar <ar...@gmail.com>
Gerrit-Reviewer: Arnab Karmakar <ar...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 17 Jul 2026 09:24:02 +0000
Gerrit-HasComments: Yes