Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13557 )
Change subject: IMPALA-8459. LocalCatalog: Allow dropping tables that fail to load ...................................................................... Patch Set 1: (4 comments) r2 fixes the comments. Will rebase and run a full test run locally as suggested by Tim http://gerrit.cloudera.org:8080/#/c/13557/1/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/13557/1/fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java@40 PS1, Line 40: * Represents a table with incomplete metadata. The metadata may be incomplete because > yea I think this only gets used by the "v1" catalog. I'll add a comment. Done http://gerrit.cloudera.org:8080/#/c/13557/1/fe/src/main/java/org/apache/impala/catalog/local/FailedLoadLocalTable.java File fe/src/main/java/org/apache/impala/catalog/local/FailedLoadLocalTable.java: http://gerrit.cloudera.org:8080/#/c/13557/1/fe/src/main/java/org/apache/impala/catalog/local/FailedLoadLocalTable.java@33 PS1, Line 33: private TableLoadingException cause_; > nit: final? Done http://gerrit.cloudera.org:8080/#/c/13557/1/fe/src/main/java/org/apache/impala/catalog/local/FailedLoadLocalTable.java@37 PS1, Line 37: cause > nit: checkNotNull()? Done http://gerrit.cloudera.org:8080/#/c/13557/1/fe/src/main/java/org/apache/impala/service/MetadataOp.java File fe/src/main/java/org/apache/impala/service/MetadataOp.java: http://gerrit.cloudera.org:8080/#/c/13557/1/fe/src/main/java/org/apache/impala/service/MetadataOp.java@317 PS1, Line 317: || table instanceof FeIncompleteTable > Can't we just override isLoaded() in FailedLoad..Table impl to make it less the problem with making 'isLoaded()' return false in the "failed table" case is that StmtMetadataLoader will then think it has to keep trying to load the table. Agree this stuff is pretty janky, mostly because the original implementation uses IncompleteTable to mean both an unloaded placeholder table and a loaded-but-failed table. Didn't want to try to refactor all of that legacy code here. -- To view, visit http://gerrit.cloudera.org:8080/13557 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia85d6b10669866dcc9f2bf7385a4775e483dd6b0 Gerrit-Change-Number: 13557 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Thu, 13 Jun 2019 20:32:36 +0000 Gerrit-HasComments: Yes
