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 16: (3 comments) http://gerrit.cloudera.org:8080/#/c/17858/15/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/15/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4341 PS15, Line 4341: Preconditions.checkState(!previousWriteIdList.isWriteIdAborted(writeId), > My thought is that only aborted state is not allowed in this method. It can ok. Please add a comment for it 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@2506 PS12, Line 2506: // Restore original config > I added an unit test for AlterPartition, which add a parameter to partition Is it because getAllWriteEventInfo() HMS api would return null writeIdList since there was no data insertion? If so, we want to have a test that asserts this behavior but your current test proposal does not do that. http://gerrit.cloudera.org:8080/#/c/17858/16/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/16/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2608 PS16, Line 2608: assertEquals(Lists.transform(FDbefore, HdfsPartition.FileDescriptor.TO_BYTES), >From what I understand, this asserts that underlying file metatdata remained >unchanged which is expected because only some parameters were added to a >partition. But this test does not assert that underlying file metadata was not >reloaded. Is that correct? -- 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: 16 Gerrit-Owner: Yu-Wen Lai <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Fucun Chu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Yu-Wen Lai <[email protected]> Gerrit-Comment-Date: Tue, 16 Nov 2021 17:58:58 +0000 Gerrit-HasComments: Yes
