Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17858 )
Change subject: IMPALA-10923: Fine grained table refreshing at partition level events for transactional tables ...................................................................... Patch Set 11: (7 comments) The patch looks pretty good now. I have some questions and minor comments below after which I would be able to +1 the patch from my side. http://gerrit.cloudera.org:8080/#/c/17858/11/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/17858/11/fe/src/main/java/org/apache/impala/catalog/Catalog.java@96 PS11, Line 96: of write ids I think it would be good to document what this map contains. Specifically, the way I understand it: This Map contains transaction to write id mapping for all the open transactions which are detected by the catalogd via events processor. The entries in the map gets cleaned up on receiving a commit transaction or abort transaction events. http://gerrit.cloudera.org:8080/#/c/17858/10/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/17858/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2924 PS10, Line 2924: please throw CatalogException here instead of RuntimeException so that the callers can check and handle it. http://gerrit.cloudera.org:8080/#/c/17858/11/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/17858/11/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2739 PS11, Line 2739: Reloading partition metadata from event: {} ({}) log statement doesn't match with the parameters. http://gerrit.cloudera.org:8080/#/c/17858/11/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2922 PS11, Line 2922: RuntimeException please change this to a checked Exception like CatalogException so that the callers can handle them. http://gerrit.cloudera.org:8080/#/c/17858/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/17858/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@529 PS11, Line 529: // Since a CommitTxnEvent could be skipped, we have to make sure no memory leak by : // clearing all txn to write ids mapping. : catalog_.clearWriteIds(); Do you think a better place for this would in the CatalogServiceCatalog#reset() method? This method gets called when the catalog starts or when user issues a invalidate metadata; command. http://gerrit.cloudera.org:8080/#/c/17858/11/fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java File fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java: http://gerrit.cloudera.org:8080/#/c/17858/11/fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java@245 PS11, Line 245: addOpenWriteId Is this method idempotent? For instance, what is the behavior if the writeId which is being added is already present in the exceptions list. Could this scenario arise when say Impala is inserting into ACID tables? The flow would be Impala will open transaction get writeId insert into table commit transaction. Now sometime later it is possible the events processor receives OPEN_TXN, ALLOC_WRITE_ID and COMMIT_TXN events. What is the behavior in such a case? http://gerrit.cloudera.org:8080/#/c/17858/11/fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java@252 PS11, Line 252: exceptions My understanding is that exceptions is sorted list. Is it guaranteed that the input writeId will be the maximum in the exceptions list? -- To view, visit http://gerrit.cloudera.org:8080/17858 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ba07c9a338a25614690e314335ee4b801486da9 Gerrit-Change-Number: 17858 Gerrit-PatchSet: 11 Gerrit-Owner: Yu-Wen Lai <yu-wen....@cloudera.com> Gerrit-Reviewer: Anonymous Coward <kis...@cloudera.com> Gerrit-Reviewer: Fucun Chu <chufu...@hotmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sourabh Goyal <soura...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Yu-Wen Lai <yu-wen....@cloudera.com> Gerrit-Comment-Date: Fri, 29 Oct 2021 21:07:05 +0000 Gerrit-HasComments: Yes
