Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/15887 )
Change subject: IMPALA-9669: Fix wrong types/comments of loaded tables/views for GET_TABLES in LocalCatalog ...................................................................... Patch Set 9: (4 comments) Thanks for the comments! Revised the patch accordingly. Also make some changes to avoid unnecessarily invalidating the table list in usual case (cachedType is null and loadedType is table). http://gerrit.cloudera.org:8080/#/c/15887/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15887/7//COMMIT_MSG@20 PS7, Line 20: This is fixed by loading table types and comments along with table names > I guess we never had SYNC_DDL semantics for GET_TABLES, did we? I.e. a tabl Yeah, SYNC_DDL only works for DDL/DMLs. To be specifit, only execDdlRequest RPCs with SYNC_DDL=true will wait for the topic update that covers the changes being sent: https://github.com/apache/impala/blob/6fcc758c1aa0b6075738edb76371a0cc43d04388/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L497 For catalog-v2, table loading is triggered by getPartialCatalogObject RPCs which is another code path. http://gerrit.cloudera.org:8080/#/c/15887/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/15887/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@721 PS6, Line 721: }); > Just a side note, if we're running with catalog_topic_mode=minimal, we coul @Vihang, table loading doesn't have SYNC_DDL option. only execDdlRequest RPCs will deal with SYNC_DDL. I plan to add a flag for IMPALA-9670 since it will increase the catalogd launch time. If the flag is turned off, we still need this... @Tim, thanks for providing the workaround! I'll add a TODO for removing this code path. http://gerrit.cloudera.org:8080/#/c/15887/7/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/15887/7/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@723 PS7, Line 723: aded at the time in catalogd, we assume that the table type is "TABLE" and : // comment is empty (See MetadataOp.getTableType and MetadataOp.getT > Do you its clearer if we say it like this? "The table list is populated bas Yeah, thanks for giving a clearer comment! http://gerrit.cloudera.org:8080/#/c/15887/7/fe/src/main/java/org/apache/impala/catalog/local/LocalIncompleteTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalIncompleteTable.java: http://gerrit.cloudera.org:8080/#/c/15887/7/fe/src/main/java/org/apache/impala/catalog/local/LocalIncompleteTable.java@31 PS7, Line 31: public class LocalIncompleteTable extends LocalTable implements FeIncompleteTable { > These can be null if it's partially loaded, right? Yes, added a Nullable annotation to emphasize this. We deal with nulls in MetadataOp.getTableType() and MetadataOp.getTableComment(). -- To view, visit http://gerrit.cloudera.org:8080/15887 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2180c603f061838347936f718cd4a0257d82e633 Gerrit-Change-Number: 15887 Gerrit-PatchSet: 9 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Wed, 20 May 2020 11:59:22 +0000 Gerrit-HasComments: Yes
