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 <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 17 Jul 2026 09:24:02 +0000 Gerrit-HasComments: Yes
