Yongzhi Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/13215 )
Change subject: IMPALA-8438: Store WriteId and ValidWriteId list for table and partition ...................................................................... Patch Set 6: (14 comments) submit patch 7 http://gerrit.cloudera.org:8080/#/c/13215/6/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/13215/6/common/thrift/CatalogObjects.thrift@479 PS6, Line 479: iff > nit: typo It means if and only if. It will not set the list when it is null, so it is right spell. http://gerrit.cloudera.org:8080/#/c/13215/6/common/thrift/CatalogObjects.thrift@482 PS6, Line 482: <open_writeids>:<abort_writeids> > nit: maybe you could explain how can we decode these parts (if they are not I will add a method to parse the string to ValidWriteIdList http://gerrit.cloudera.org:8080/#/c/13215/6/common/thrift/CatalogObjects.thrift@482 PS6, Line 482: // <table_name>:<highwatermark>:<minOpenWriteId>:<open_writeids>:<abort_writeids> > Are we guaranteed that Hive team will keep this string backward compatible? We use their function do the converts (writeToString, fromTheString). Hive uses in their clients too. http://gerrit.cloudera.org:8080/#/c/13215/6/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/13215/6/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@374 PS6, Line 374: * @return the list of valid write IDs for the table in a string > Nit: or null if there are no validWriteIds Done http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@388 PS6, Line 388: public static long getHighestWriteId(String validWriteIds) > This method is no longer used. Let's remove it until we have a use Done http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@399 PS6, Line 399: which is missing : * in HMS 2 > nit: maybe instead of writing that it is missing in HMS 2, you could briefl Done http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@407 PS6, Line 407: which is missing : * in HMS 2 > nit: same as above Done http://gerrit.cloudera.org:8080/#/c/13215/6/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/13215/6/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@236 PS6, Line 236: StringBuilder validIdsBuf = new StringBuilder("Loaded ValidWriteIdLists: "); > For my understanding, how do we use timeline? It is a little odd to have time at the end. Use timeline to put the writeID info into profile. we may improve timeline later or do you have better suggestions? http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java File fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java: http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java@111 PS6, Line 111: public long getWriteId() { > maybe we can implement these as default methods in the FeTable interface? This DataSourceTable class extends Table class and implements FeDataSourceTable. Put a default implementation for FeTable will make it more complicated. And because Table already implements these methods, DataSourceTable has to override here(I assume DataSourceTable does not support writeId related operations). http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@649 PS6, Line 649: writeId_ = msPartition != null ? > Nit: Handle null case in shim so that every call does not have to handle it Done http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@1026 PS6, Line 1026: } > Nit: Use ternary or put else in the above line Done http://gerrit.cloudera.org:8080/#/c/13215/6/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/13215/6/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@924 PS6, Line 924: //TODO put the writeID load here for now. > Can you rephrase this to explain the _future_ state, not the current state? Done http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/main/java/org/apache/impala/catalog/Table.java@287 PS6, Line 287: LOG.warn("Could not get valid writeIds for: " + tblFullName, e); > shouldn't we be passing this through as a tableloadingexception of some kin Done http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java File fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java: http://gerrit.cloudera.org:8080/#/c/13215/6/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java@99 PS6, Line 99: <= MetastoreShim.getHighestWriteId(t.getValidWriteIds())); > Instead can we assert on t.getValidWriteIds().isWriteIdValid(t.getWriteId() Done -- To view, visit http://gerrit.cloudera.org:8080/13215 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6edbd64424edf0ba88af110ab8b958a1966b8b54 Gerrit-Change-Number: 13215 Gerrit-PatchSet: 6 Gerrit-Owner: Yongzhi Chen <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sudhanshu Arora <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Yongzhi Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 07 May 2019 17:50:29 +0000 Gerrit-HasComments: Yes
