Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19838 )
Change subject: IMPALA-11534: Skip reloading file metadata for some ALTER_TABLE events ...................................................................... Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/19838/2/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/19838/2/be/src/catalog/catalog-server.cc@136 PS2, Line 136: "compactorthreshold.hive.compactor.delta.num.threshold Why should Impala reload the table when these compactor props change? http://gerrit.cloudera.org:8080/#/c/19838/2/be/src/catalog/catalog-server.cc@136 PS2, Line 136: "compactorthreshold.hive.compactor.delta.num.threshold, " : "compactorthreshold.hive.compactor.delta.pct.threshold, transactional", We could add an Impala specific property here to give users the ability to force reload (without restarting the cluster and changing flags or messing with flags that have other uses). It could be also useful in tests. For example: impala.metadata_reload_prop http://gerrit.cloudera.org:8080/#/c/19838/2/be/src/catalog/catalog-server.cc@139 PS2, Line 139: "are supposed to reload file metadata these properties are changed."); nit: missing "when"? http://gerrit.cloudera.org:8080/#/c/19838/2/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/19838/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@326 PS2, Line 326: ; nit: members postfixed with _ http://gerrit.cloudera.org:8080/#/c/19838/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3891 PS2, Line 3891: realoding typo http://gerrit.cloudera.org:8080/#/c/19838/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3896 PS2, Line 3896: isSkipFileMetadataReload naming: maybe canSkipFileMetadataReload would be clearer? http://gerrit.cloudera.org:8080/#/c/19838/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3900 PS2, Line 3900: return false; The return value is not clear to me: does this mean that we should never skip reload is whitelistedTblProperties is empty or that we should always skip it? http://gerrit.cloudera.org:8080/#/c/19838/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3904 PS2, Line 3904: if (isFieldSchemaChanged(beforeTable, afterTable) || : !beforeTable.getOwner().equals(afterTable.getOwner()) || : !isCustomTblPropsChanged(beforeTable, afterTable)) { : return true; : } again, I don't understand the return value - if any of the conditions above is true, doesn't it mean that we have to reload the table? http://gerrit.cloudera.org:8080/#/c/19838/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3942 PS2, Line 3942: equalsIgnoreCase I am not 100% sure that we can ignore the case. I can imagine a properties like paths or base64 encoded values where the case does matter. http://gerrit.cloudera.org:8080/#/c/19838/2/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/19838/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2824 PS2, Line 2824: assertEquals(fileMetadataLoadAfter, fileMetadataLoadBefore); It would be great to test that some table property changes actually lead to metadata load. For example the property I recommended in catalog-server.cc could be used to test that: - adding leads to reload - removing leads to reload - changing leads to reload - setting to the same value doesn't lead to reload -- To view, visit http://gerrit.cloudera.org:8080/19838 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia66b96a7c4b7f50fbf46b2e02296cd29a47347b6 Gerrit-Change-Number: 19838 Gerrit-PatchSet: 2 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Fri, 12 May 2023 10:52:05 +0000 Gerrit-HasComments: Yes
