Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21478 )
Change subject: IMPALA-13120: Load failed table without need for manual invalidate ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/21478/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/21478/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3703 PS4, Line 3703: Preconditions.checkState(table.isLoaded()); > It happened due to a bug introduced in patchset-2. Ack http://gerrit.cloudera.org:8080/#/c/21478/4/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java File fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/21478/4/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@170 PS4, Line 170: in the previous load : // attempt nit: do you mean previous queries? 'attempt' sounds like an iteration in the loop of L207. http://gerrit.cloudera.org:8080/#/c/21478/4/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@173 PS4, Line 173: tablesToLoad nit: Can we rename this to something like 'missingTblsSnapshot' or 'originalMissingTbls'? This seems to be a snapshot of the initial state of the missing tables + LoadFailedByRecoverableError tables. If the table is loaded again and still failed by recoverable errors, it won't be added into this map. http://gerrit.cloudera.org:8080/#/c/21478/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/21478/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2470 PS4, Line 2470: // if no validWriteIdList is provided, we return the tbl if its loaded : // In the external front end use case it is possible that an external table might : // have validWriteIdList, so we can simply ignore this value if table is external nit: let's move these comments to L2480. http://gerrit.cloudera.org:8080/#/c/21478/4/fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java File fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java: http://gerrit.cloudera.org:8080/#/c/21478/4/fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java@95 PS4, Line 95: String metastoreConnectionError = "Could not connect to meta store"; nit: can we make this a constant? http://gerrit.cloudera.org:8080/#/c/21478/4/tests/custom_cluster/test_catalog_hms_failures.py File tests/custom_cluster/test_catalog_hms_failures.py: http://gerrit.cloudera.org:8080/#/c/21478/4/tests/custom_cluster/test_catalog_hms_failures.py@90 PS4, Line 90: --catalog_topic_mode=minimal nit: --catalog_topic_mode is not used in impalad. We can remove this. http://gerrit.cloudera.org:8080/#/c/21478/4/tests/custom_cluster/test_catalog_hms_failures.py@109 PS4, Line 109: print(str(e)) nit: remove print() ? -- To view, visit http://gerrit.cloudera.org:8080/21478 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia882fdd865ef716351be7f1eaf203a9fb04c1c15 Gerrit-Change-Number: 21478 Gerrit-PatchSet: 4 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Thu, 27 Jun 2024 02:54:32 +0000 Gerrit-HasComments: Yes
