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 3: (20 comments) Patch 3 addressed review issues. http://gerrit.cloudera.org:8080/#/c/13215/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13215/2//COMMIT_MSG@7 PS2, Line 7: > nit: we generally don't add a . to the commit message title Done http://gerrit.cloudera.org:8080/#/c/13215/2//COMMIT_MSG@8 PS2, Line 8: > nit: leave an empty line after the title in the commit message Done http://gerrit.cloudera.org:8080/#/c/13215/2//COMMIT_MSG@9 PS2, Line 9: > nit: functions Done http://gerrit.cloudera.org:8080/#/c/13215/2//COMMIT_MSG@10 PS2, Line 10: Add MetastoreShim fucntions to support HMS3 only functions. > nit: extra line before Tests Done http://gerrit.cloudera.org:8080/#/c/13215/2/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/13215/2/common/thrift/CatalogObjects.thrift@307 PS2, Line 307: // For acid table, store last committed write id > nit: capital F + ending . Done http://gerrit.cloudera.org:8080/#/c/13215/2/common/thrift/CatalogObjects.thrift@479 PS2, Line 479: // Set iff this is an acid table. The valid write ids list > Can you add more information about the format, e.g. an example? Done http://gerrit.cloudera.org:8080/#/c/13215/2/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/2/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@371 PS2, Line 371: * Get valid write ids for the acid table : * @param client the client to access HMS : * @param tableFullName the name for the table : * @param wrtId the write id to get the corresponding txn : * @return the list of valid write IDs for the table in a string > nit: try to be consistent with ending . Done http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@388 PS2, Line 388: /** : * Get highest writeId in a given validwriteidlist : * @param validWriteIds ValidWriteIdList object in String : * @return highest writeId : */ > nit: try to be consistent with ending . Done http://gerrit.cloudera.org:8080/#/c/13215/2/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/2/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@235 PS2, Line 235: for (FeTable iTbl : loadedTbls_.values()) { > optional: the logging could be moved to a separate function like LogWriteId Done http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@240 PS2, Line 240: } > nit: missing space? Done http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@245 PS2, Line 245: t > nit: extra space? Done http://gerrit.cloudera.org:8080/#/c/13215/2/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/2/fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java@172 PS2, Line 172: > nit: . not needed Done http://gerrit.cloudera.org:8080/#/c/13215/2/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/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@617 PS2, Line 617: For Acid table > nit: consistency: "For acid tables." Done http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@1023 PS2, Line 1023: if (thriftPartition.isSetWrite_id()) { > nit: we generally add braces if the "if" doesn't fit to one line Done http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@1025 PS2, Line 1025: } > nit: braces or one line Done http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@1069 PS2, Line 1069: > @Override Done http://gerrit.cloudera.org:8080/#/c/13215/2/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/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@924 PS2, Line 924: //TODO put the writeID load here for now. : loadValidWriteIdList(client); > Can you mention this TODO in the commit message too? Done http://gerrit.cloudera.org:8080/#/c/13215/2/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/2/fe/src/main/java/org/apache/impala/catalog/Table.java@120 PS2, Line 120: // Last commited write id for this table > nit: missing . Done http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/catalog/Table.java@286 PS2, Line 286: the table > It could be also useful to log the results if LOG.isTraceEnabled(). Done http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/catalog/Table.java@392 PS2, Line 392: // Default to READ_WRITE access if the field is not set. : accessLevel_ = thriftTable.isSetAccess_level() ? thriftTable.getAccess_level() : : > What writeId_ == 0 and writeId_ > 0 mean here? If it is > 0, then we always It is majorly for partitioned tables. Their table writeId (which is often 0) can not be used to get ValidWriteIDs(cause exception). Which value should be here is undecided yet. -- 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: 3 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: Thu, 02 May 2019 23:13:02 +0000 Gerrit-HasComments: Yes
