Csaba Ringhofer 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 partition. This happens when table load metadata from HMS Add MetastoreShim fucntions to support HMS3 only function. Tests: Manually tests HMS2 and HMS3, using log files to check. Unit tests ...................................................................... Patch Set 2: (25 comments) Most of my comments are nits about conforming to Impala FE conventions. 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 http://gerrit.cloudera.org:8080/#/c/13215/2//COMMIT_MSG@8 PS2, Line 8: This happens when table load metadata from HMS nit: leave an empty line after the title in the commit message http://gerrit.cloudera.org:8080/#/c/13215/2//COMMIT_MSG@9 PS2, Line 9: fucntions type: functions http://gerrit.cloudera.org:8080/#/c/13215/2//COMMIT_MSG@9 PS2, Line 9: n nit: functions http://gerrit.cloudera.org:8080/#/c/13215/2//COMMIT_MSG@10 PS2, Line 10: Tests: nit: extra line before Tests 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 . 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? + nit: ending . 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 . Note that we generally do not use Javadoc in Impala but I am not against using it. 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 . 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 LogWriteIds() http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@240 PS2, Line 240: LOG.info("current write id is:" + iTbl.getWriteId()); nit: missing space? http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@245 PS2, Line 245: nit: extra space? 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 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." 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 http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@1025 PS2, Line 1025: else nit: braces or one line http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@1069 PS2, Line 1069: public long getWriteId() @Override 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? 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 . http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/catalog/Table.java@121 PS2, Line 121: protected long writeId_; A comment about the special role of 0 and -1 would be nice. http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/catalog/Table.java@123 PS2, Line 123: v V + missing . http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/catalog/Table.java@286 PS2, Line 286: validwriteids It could be also useful to log the results if LOG.isTraceEnabled(). + nit: missing spaces? http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/main/java/org/apache/impala/catalog/Table.java@392 PS2, Line 392: if (writeId_ == 0 && validWrtIds_ != null) { : writeId_ = MetastoreShim.getHighestWriteId(validWrtIds_); : } What writeId_ == 0 and writeId_ > 0 mean here? If it is > 0, then we always trust that it is the correct writeId that would be calculated from validWrtIds_? http://gerrit.cloudera.org:8080/#/c/13215/2/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/2/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java@121 PS2, Line 121: nit: unnecessary line http://gerrit.cloudera.org:8080/#/c/13215/2/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java@234 PS2, Line 234: testLoadAcidTables("select * from acid.insert_only_no_partitions"); : testLoadAcidTables("select * from acid.insert_only_with_partitions") These tables are not yet included. Can you mention this as a TODO in the commit message, and maybe add a link for the source? -- 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: 2 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-Comment-Date: Thu, 02 May 2019 19:54:26 +0000 Gerrit-HasComments: Yes
