Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/15818 )
Change subject: IMPALA-9512: Full ACID Milestone 2: Validate rows against the valid write id list ...................................................................... Patch Set 13: (7 comments) Thanks Quanlong for the comments! http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@611 PS13, Line 611: new BitSet() > nit: move this to the else-clase? Done http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2096 PS13, Line 2096: sw.elapsed(TimeUnit.NANOSECONDS) > I think we should return this and add it into storageMetadataLoadTime_ To me it seems like storageMetadataLoadTime_ is only for time spent in file system related operations. E.g. it doesn't accumulate time spent in loadSchema(), loadAllColumnStats(), etc. Also, in updatePartitionsFromHms() it only measures the load time of file metadata. http://gerrit.cloudera.org:8080/#/c/15818/13/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/15818/13/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@45 PS13, Line 45: import org.apache.hadoop.hive.metastore.api.TableMeta; > nit: unused import Done http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@62 PS13, Line 62: import org.apache.impala.compat.MetastoreShim; > nit: unused import Done http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@405 PS13, Line 405: throw new NotImplementedException( > We can call HMS API getValidWriteIds(fullTableName) to get this directly. In CatalogdMetaProvider we load validWriteIdList together with catalog version. AFAICT this guarantees that the valid write id list and the table metadata will be consistent. I'm afraid of just independently loading the valid write id list here because it might introduce subtle bugs, like the valid write id list won't be consistent with the loaded files. I also don't know the current state and usability of DirectMetaProvider. I see some parts of it are already used by CatalogdMetaProvider, but some functions are not complete. E.g. the comments in loadFileMetadata() says that DirectMetaProvider is not yet supported, and it also doesn't handle transactional tables correctly. So I'd probably just throw this exception for now. http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java: http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@331 PS13, Line 331: if (ref_ != null) { > Could you leave a comment about when ref_ will be null? It confused me at f Done. Yeah, I've hit this with CTAS statements. http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java: http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@23 PS13, Line 23: import org.apache.commons.lang.NotImplementedException; > nit: unused import Done -- To view, visit http://gerrit.cloudera.org:8080/15818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5ed74585a2d73ebbcee763b0545be4412926299d Gerrit-Change-Number: 15818 Gerrit-PatchSet: 13 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 19 May 2020 11:07:49 +0000 Gerrit-HasComments: Yes
