Sourabh Goyal 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 14: (8 comments) http://gerrit.cloudera.org:8080/#/c/17858/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/17858/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3558 PS12, Line 3558: tbl = getTable(dbName, tblName); > In this case, that would make sense to load the table before processing All Works for me. We can take it up in a follow up patch. However, I would like Vihang (since he has already reviewed the PR) to take a look at this to make sure we are not missing any issue here. http://gerrit.cloudera.org:8080/#/c/17858/13/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/17858/13/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3560 PS13, Line 3560: LOG.debug("Not adding write ids since database {} was not found", dbName); nit: add more details in the log message like table name, event id being processed. http://gerrit.cloudera.org:8080/#/c/17858/12/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/12/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2737 PS12, Line 2737: Preconditions.checkArgument(partsFromEvent != null > I added the check in reloadPartitions because there's where we modify the t reloadPartitionsFromEvent is a public api of HdfsTable and anyone can call this api as long as the caller has access to table object. Preconditions check will ensure that any other user of this api in future would have to acquire write lock on the table first 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@252 PS11, Line 252: exceptions.add(currentId); > I don't think we need to rely on the caller. AbortedBits need to be set onl Looked at the implementation of BitSet.get() and I think the following sequence already works but can you just reconfirm? If it does work, I am fine with current implementation as well 1. addOpenWriteId(writeId) // add a new writeId which is greater than current highWatermark 2. isWriteIdOpen(writeId) -> should return true http://gerrit.cloudera.org:8080/#/c/17858/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/17858/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4342 PS12, Line 4342: part.setWriteId(writeId); > I assume this method is called by a commit event and it is impossible to ha No HMS won't be wrong. But adding a check enforce the expected behavior of addCommittedWriteIdsAndReloadPartitionsIfExist and in my opinion it is always good to enforce stricter checks to avoid any abuse (knowingly or unknowingly).. http://gerrit.cloudera.org:8080/#/c/17858/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/17858/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4368 PS13, Line 4368: hdfsTable.setValidWriteIds(previousWriteIdList); nit: Would be good to add a log message with details about the rollback. http://gerrit.cloudera.org:8080/#/c/17858/12/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/17858/12/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2487 PS12, Line 2487: BackendConfig.create(origCfg); nit: Original config should be restored in finally block http://gerrit.cloudera.org:8080/#/c/17858/12/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2818 PS12, Line 2818: params.put("transactional", "true"); > Could you elaborate this? A transactional table must be a managed table. Wh Instead of creating a new method createTransactionalTable, we can enhance getTestTable() by passing an additional parameter which accepts a table type. Check this out for example: https://gerrit.cloudera.org/c/17859/27/fe/src/test/java/org/apache/impala/catalog/MetastoreApiTestUtils.java#78 -- 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: 14 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: Tue, 09 Nov 2021 18:20:36 +0000 Gerrit-HasComments: Yes
