Vihang Karajgaonkar 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 8: (12 comments) Took a first pass at this. I mostly left some nits and code style comments. I will take another pass diving more into the functionality or cases which might be missing in some time. 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@26 PS8, Line 26: is nit, remove http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoadOpts.java@27 PS8, Line 27: incorrectness correctness issues? http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoadOpts.java@38 PS8, Line 38: public nit, add a new line before the method. 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: Reloads file metadata of all : * HdfsPartitions may be a better wording could be "This method also reloads file metadata of all the partitions for the given partNames" http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2808 PS8, Line 2808: nit, add a new line http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2845 PS8, Line 2845: partBuilder.isMarkedCached() Why do we need this condition? http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2858 PS8, Line 2858: partBuildersFileMetadataRefresh.size() > 0 nit, !partBuildersFileMetadataRefresh.isEmpty() 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 comment correctly. See other methods in this file for example. 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. http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2817 PS8, Line 2817: assertTrue nit, change to assertEquals(fileMetadataLoadBefore, fileMetadataLoadAfter). The assertion error automatically prints the expected and current value. http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2874 PS8, Line 2874: assertTrue nit, assertEquals(fileMetadataLoadBefore + 1, fileMetadataLoadAfter) look much more readable to me. http://gerrit.cloudera.org:8080/#/c/18083/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2878 PS8, Line 2878: // drop partition from cache and trigger reloadPartitions with : // FileMetadataLoadOpts.LOAD_IF_SD_CHANGED may be a more real world way for writing this test would be to add partition from Impala and then drop it from hiveClient. -- 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: 8 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: Thu, 06 Jan 2022 19:18:59 +0000 Gerrit-HasComments: Yes
