Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16008 )
Change subject: IMPALA-9791: Support validWriteIdList in getPartialCatalogObject API ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/16008/8/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/16008/8/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java@83 PS8, Line 83: try (MetaStoreClient client = MetaStoreClientPool.get().getClient()) { : try { nit: can we have a single try-catch? http://gerrit.cloudera.org:8080/#/c/16008/8/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java@101 PS8, Line 101: FileMetadataLoader loader = new FileMetadataLoader(e.getKey(), : Utils.shouldRecursivelyListPartitions(table), oldFds, table.getHostIndex(), : validTxnList, writeIdList, e.getValue().get(0).getFileFormat()); If 'validTxnList' is loaded after a while since 'writeIdList' is fetched by the client, then it might load a compacted directory that has contents we shouldn't see based on the write id list. So we should either do the strict check, or make the client provide a 'validTxnList' alongside with the write id list. You already mentioned the need for the strict check in the fallback path in a previous comment, but I think the current code doesn't have it. Or maybe I'm missing something. To detect removed directories, we should check whether we have directories/files for all the committed write ids. If the files gets deleted shortly after, the backend would throw an error. However, as we already talked about it out of band, the ideal solution would be to open a transaction and put a SHARED_READ lock on the table. http://gerrit.cloudera.org:8080/#/c/16008/8/fe/src/main/java/org/apache/impala/util/AcidUtils.java File fe/src/main/java/org/apache/impala/util/AcidUtils.java: http://gerrit.cloudera.org:8080/#/c/16008/8/fe/src/main/java/org/apache/impala/util/AcidUtils.java@432 PS8, Line 432: Catalog Thanks for migrating this to CatalogException! http://gerrit.cloudera.org:8080/#/c/16008/8/fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java File fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java: http://gerrit.cloudera.org:8080/#/c/16008/8/fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java@145 PS8, Line 145: MetaE nit: CatalogException -- To view, visit http://gerrit.cloudera.org:8080/16008 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ied2c7c3cb2009c407e8fbc3af4722b0d34f57c4a Gerrit-Change-Number: 16008 Gerrit-PatchSet: 8 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 05 Jun 2020 13:32:18 +0000 Gerrit-HasComments: Yes
