Todd Lipcon 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 8: (13 comments) http://gerrit.cloudera.org:8080/#/c/13215/8/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/8/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@255 PS8, Line 255: return -1L; agree with Sudanshu's earlier comments -- I think it's better to throw UnsupportedOperationException here, or to make these functions return Optional<Long> (or just Long with null indicating unset) so that we don't accidentally end up treating -1 as a valid write ID anywhere. http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-2/java/org/apache/impala/compat/ValidWriteIdList.java File fe/src/compat-hive-2/java/org/apache/impala/compat/ValidWriteIdList.java: http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-2/java/org/apache/impala/compat/ValidWriteIdList.java@2 PS8, Line 2: // // or more contributor license agreements. See the NOTICE file nit: double // here http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-2/java/org/apache/impala/compat/ValidWriteIdList.java@18 PS8, Line 18: Doesn't this need to be in the same package as the Hive ValidWriteIdClass so that the shim methods return the same type? http://gerrit.cloudera.org:8080/#/c/13215/8/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/8/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@387 PS8, Line 387: public static String fetchValidWriteIdsString(IMetaStoreClient client, why do we need this class if we already have the above one, and the shim class for ValidWriteIds? http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@399 PS8, Line 399: if (validWriteIds != null) { Why check against null here instead of Preconditions.checkNotNull()? http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@412 PS8, Line 412: return partition == null ? -1L : partition.getWriteId(); Same -- we shouldn't be checking for null and papering over it. It should be an error to pass null to these functions. http://gerrit.cloudera.org:8080/#/c/13215/8/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/8/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@238 PS8, Line 238: System.lineSeparator nit: we just use \n elsewhere http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@243 PS8, Line 243: validIdsBuf.append(" "); why this extra whitespace? http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java File fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java: http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java@172 PS8, Line 172: * @return the writeId stored in hms for the partition Per comments elsewhere, I think we should either change these to Long (to allow for null when we have no write id known) or at least clearly document than '-1 indicates unknown' so that callers know they need to handle that. (I see you documented that on the member variable in one spot, but I think it's also important to document on these cross-system interfaces) http://gerrit.cloudera.org:8080/#/c/13215/8/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/8/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@618 PS8, Line 618: //-1 means writeId_ is irrelevant(not supported). nit: space after // http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@1022 PS8, Line 1022: : if (thriftPartition.isSetWrite_id()) { : partition.writeId_ = thriftPartition.getWrite_id(); : } else { : partition.writeId_ = -1l; : } Use a ternary? http://gerrit.cloudera.org:8080/#/c/13215/8/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/8/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@924 PS8, Line 924: //TODO writeIDs may also be loaded in other code paths. I'm still not quite sure I understand this TODO http://gerrit.cloudera.org:8080/#/c/13215/8/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/8/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java@235 PS8, Line 235: testLoadAcidTables("select * from acid.insert_only_no_partitions"); We need to get this into the data loading setup before we can commit this change -- 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: 8 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: Mon, 13 May 2019 22:31:50 +0000 Gerrit-HasComments: Yes
