Yongzhi Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/13215 )
Change subject: IMPALA-8438: Store WriteId and valid writeId list for table and partitio ...................................................................... Patch Set 4: (16 comments) http://gerrit.cloudera.org:8080/#/c/13215/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13215/3//COMMIT_MSG@12 PS3, Line 12: Tests: > I think if we add some of this info to the profile or to EXPLAIN output we Done http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@238 PS3, Line 238: return -1L; > nit: please use capital L so this doesn't look like -11. (same below) Done http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@258 PS3, Line 258: public static long getMajorVersion() { > can we rename this to getMajorVersion or something since it's not the full Done http://gerrit.cloudera.org:8080/#/c/13215/3/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/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@374 PS3, Line 374: * @return the list of valid write IDs for the table in a string > I had to trace my way through Hive code to understand what this meant. My u We do not have use case, will remove it. http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@378 PS3, Line 378: lidWriteIdLi > can we use Optional<Long> here? writeId is removed http://gerrit.cloudera.org:8080/#/c/13215/3/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/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@236 PS3, Line 236: StringBuilder validIdsBuf = new StringBuilder("Loaded ValidWriteIdLists: "); > How about putting this into the query timeline event marked above? This way Done http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@285 PS3, Line 285: * Returns the set of tables whose metadata needs to be loaded for the analysis of the > this won't work with LocalCatalog mode enabled. Probably best to just remov Removed the function http://gerrit.cloudera.org:8080/#/c/13215/3/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/3/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@617 PS3, Line 617: // The last committed write ID which modified this partition. > can you be more clear here? eg "The last committed write ID which modified Done http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@618 PS3, Line 618: //-1 means writeId_ is irrelevant(not supported). > -1L Done http://gerrit.cloudera.org:8080/#/c/13215/3/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/3/fe/src/main/java/org/apache/impala/catalog/Table.java@120 PS3, Line 120: // Valid write id list for this table > The way this got implemented, this is just a high watermark write id, which The high water mark can help us get a list of valid writeIDs When apply on a ValidWriteIdList /** * Find out if a range of write ids are valid. Note that valid may have different meanings * for different implementations, as some will only want to see committed transactions and some * both committed and aborted. * @param minWriteId minimum write ID to look for, inclusive * @param maxWriteId maximum write ID to look for, inclusive * @return Indicate whether none, some, or all of these transactions are valid. */ RangeResponse isWriteIdRangeValid(long minWriteId, long maxWriteId); But the maxWriteId may comes from transaction manager? I will remove the writeId for table by following your suggestion as we can easily get from stored ValidWriteIdList http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/main/java/org/apache/impala/catalog/Table.java@285 PS3, Line 285: writeIds = MetastoreShim.fetchValidWriteIds(client, tblFullName); > per note elsewhere, I don't think we have a good use for this Done http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/main/java/org/apache/impala/catalog/Table.java@289 PS3, Line 289: } > getFullName() Done http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/main/java/org/apache/impala/catalog/Table.java@290 PS3, Line 290: .isTra > nit: don't abbreviate Done http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/main/java/org/apache/impala/catalog/Table.java@298 PS3, Line 298: * @param client the client to access HMS > I don't think you need to check for null here, since null will append to a Done http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/main/java/org/apache/impala/catalog/Table.java@311 PS3, Line 311: > maybe I misunderstood what this gets used for, but I don't think this has t Done http://gerrit.cloudera.org:8080/#/c/13215/3/fe/src/main/java/org/apache/impala/catalog/Table.java@314 PS3, Line 314: table = new View(msTbl, db, msTbl.getTableName(), msTbl.getOwner()); > I'm not sure this is useful. The high watermark doesn't tell us the latest 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: 4 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-Comment-Date: Fri, 03 May 2019 19:21:17 +0000 Gerrit-HasComments: Yes
