Sourabh Goyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/18083 )
Change subject: IMPALA-11050: Skip filemetadata reloading in processing AlterPartition event from event processor ...................................................................... Patch Set 9: (10 comments) http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoadOpts.java File fe/src/main/java/org/apache/impala/catalog/FileMetadataLoadOpts.java: http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoadOpts.java@27 PS8, Line 27: rectness issu > correctness issues? Yes I meant that. Will fix it. http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoadOpts.java@38 PS8, Line 38: > nit, add a new line before the method. Ack http://gerrit.cloudera.org:8080/#/c/18083/8/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/18083/8/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2705 PS8, Line 2705: This method also reloads file metadata : * of all the par > may be a better wording could be "This method also reloads file metadata of Yes, this is better. Will fix it. http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2845 PS8, Line 2845: oldPartition == null || !old > Why do we need this condition? The thought process was: If a partition has a cache directive set in the HDFS, then we always reload the file metadata for such partition so that file blocks info is always up to date. http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2858 PS8, Line 2858: > nit, !partBuildersFileMetadataRefresh.isEmpty() Ack http://gerrit.cloudera.org:8080/#/c/18083/8/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/18083/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2714 PS8, Line 2714: /* > please use java doc style block comment (/**) so that IDE renders the comme Sure. http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2778 PS8, Line 2778: > nit, please use java doc style block comments. Ack http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2817 PS8, Line 2817: long fileM > nit, change to assertEquals(fileMetadataLoadBefore, fileMetadataLoadAfter). Yes, that is better. Thanks for the suggestion. http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2874 PS8, Line 2874: > nit, assertEquals(fileMetadataLoadBefore + 1, fileMetadataLoadAfter) look m Ack http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2878 PS8, Line 2878: String partName = FeCatalogUtils.getPartitionName(tbl, partVals.get(0)); : List<HdfsPartition> partitionsToDrop = > may be a more real world way for writing this test would be to add partitio The scenario I wanted to test was - a partition doesn't exist in HdfsTable but is present in HMS. In such case, the expected behavior is that tbl.reloadPartitions() for such partition should *not* skip file metadata reload even if FileMetadataLoadOpts is LOAD_IF_SD_CHANGED. Therefore I deleted an existing partition from table cache (but not from HMS) We could probably have tested it the other way i.e create a partition from hiveClient . Then try to load it back in table cache and assert that file metadata is also reloaded irrespective of the FileMetadataLoadOpts value. Is the later approach better? -- To view, visit http://gerrit.cloudera.org:8080/18083 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I238b169f7f1122c62cbeb1434dbb675629a1e5f2 Gerrit-Change-Number: 18083 Gerrit-PatchSet: 9 Gerrit-Owner: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Yu-Wen Lai <[email protected]> Gerrit-Comment-Date: Fri, 07 Jan 2022 13:05:17 +0000 Gerrit-HasComments: Yes
