Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21019 )
Change subject: IMPALA-12487: Skip reloading file metadata for ALTER_TABLE events with trivial changes in StorageDescriptor ...................................................................... Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/21019/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21019/6//COMMIT_MSG@14 PS6, Line 14: =true nit: remove "=true" ? http://gerrit.cloudera.org:8080/#/c/21019/6//COMMIT_MSG@15 PS6, Line 15: Also introduced a small : optimization to skip reloading of table schema nit: this is stale now http://gerrit.cloudera.org:8080/#/c/21019/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/21019/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1936 PS6, Line 1936: "whitelisted config: {}, value before: {}, value after: {}", dbName_, nit: let's also add "So file metadata should be reloaded" here. http://gerrit.cloudera.org:8080/#/c/21019/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1971 PS6, Line 1971: return false; So we return false for other unknown cases. In the future if a new field is added to the definition of SD (e.g. in a higher Hive version) and the change of it is non-trivial (requires file metadata reloading), it will be skipped here (considered as trivial change). This is the case mentioned by Csaba that we need to address. As this method is only used as !isNonTrivialSdPropsChanged() at line 1886, we can convert it to isTrivialSdPropsChanged() and only return true for known cases (i.e. changes on location, input/output format, serde, storedAsSubDirectories). The difference is that canSkipFileMetadataReload() will only return true for known cases. http://gerrit.cloudera.org:8080/#/c/21019/6/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/21019/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3050 PS6, Line 3050: assertNotEquals(fileMetadataLoadAfter + 1, fileMetadataLoadBefore); Why do we change this? fileMetadataLoadAfter >= fileMetadataLoadBefore is always true, so fileMetadataLoadAfter + 1 != fileMetadataLoadBefore is always true. The assertion becomes meaningless. Do you want to use assertEquals(fileMetadataLoadAfter, fileMetadataLoadBefore + 1) instead? -- To view, visit http://gerrit.cloudera.org:8080/21019 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6fd9a9504bf93d2529dc7accbf436ad83e51d8ac Gerrit-Change-Number: 21019 Gerrit-PatchSet: 6 Gerrit-Owner: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <k.venureddy2...@gmail.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Wed, 13 Mar 2024 11:13:15 +0000 Gerrit-HasComments: Yes
