Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24711 )
Change subject: IMPALA-15234: Add a way to clear the HBO cache ...................................................................... Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/24711/8/fe/src/main/java/org/apache/impala/service/HistoricalStats.java File fe/src/main/java/org/apache/impala/service/HistoricalStats.java: http://gerrit.cloudera.org:8080/#/c/24711/8/fe/src/main/java/org/apache/impala/service/HistoricalStats.java@219 PS8, Line 219: What is in the cache when this runs, and no more: nit: It's unclear to me what this means. Could you reword this? http://gerrit.cloudera.org:8080/#/c/24711/8/fe/src/main/java/org/apache/impala/service/HistoricalStats.java@220 PS8, Line 220: without holding anything nit: do you mean "without holding any locks"? http://gerrit.cloudera.org:8080/#/c/24711/8/fe/src/main/java/org/apache/impala/service/HistoricalStats.java@221 PS8, Line 221: * there says, so a query unregistering at that moment can store a run after the clear nit: "as the TODO there says" sounds highly conversational. How about making this more formal? E.g. * Note on concurrency: This method only clears what is currently in the cache. * Because {@link #writePlanNodeStats()} modifies cache entries without locks * (as noted in its TODO), a query unregistering concurrently might store a run * immediately after this clear operation returns. Fixing this race condition * should be handled at the writer level rather than here. http://gerrit.cloudera.org:8080/#/c/24711/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/24711/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@4978 PS8, Line 4978: public void TestClearHboStatsPrivileges() throws ImpalaException { It's strange to put authorization tests here. Do we really need this test? I think AuthorizationStmtTest.testClearHboStats() already covers this. http://gerrit.cloudera.org:8080/#/c/24711/8/tests/query_test/test_hbo.py File tests/query_test/test_hbo.py: http://gerrit.cloudera.org:8080/#/c/24711/8/tests/query_test/test_hbo.py@197 PS8, Line 197: self.execute_query(": clear_hbo_stats()") nit: can we verify the response like above? -- To view, visit http://gerrit.cloudera.org:8080/24711 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib58305e96f05ba49eb03d935765620915964308a Gerrit-Change-Number: 24711 Gerrit-PatchSet: 8 Gerrit-Owner: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 08 Sep 2026 12:23:40 +0000 Gerrit-HasComments: Yes
