Sai Hemanth Gantasala 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 3: (9 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: "to whitelist the table properties that are supposed t > Why should Impala reload the table when these compactor props change? Actually both the compaction-related properties are not required, since hive is now throwing "commit_compaction _event" for any change after compaction. http://gerrit.cloudera.org:8080/#/c/19838/2/be/src/catalog/catalog-server.cc@136 PS2, Line 136: "to whitelist the table properties that are supposed to refresh file metadata" : "when these properties are changed."); > We could add an Impala specific property here to give users the ability to I see that there are some query options that can be modified at run time. But I cannot find an example in catalogD configs that can be modified at runtime. Can you please give some pointers on how to achieve this? http://gerrit.cloudera.org:8080/#/c/19838/2/be/src/catalog/catalog-server.cc@139 PS2, Line 139: DECLARE_string(state_store_host); > nit: missing "when"? Ack 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 _ Ack http://gerrit.cloudera.org:8080/#/c/19838/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3891 PS2, Line 3891: reloading > typo Ack http://gerrit.cloudera.org:8080/#/c/19838/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3896 PS2, Line 3896: canSkipFileMetadataReloa > naming: maybe canSkipFileMetadataReload would be clearer? Ack 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: If the whitelisted config is empty, we'll never skip file reload. The reason I'm doing this is because if something unexpected happens in the customer's env, clearing this config should work the way it is working currently. 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 if any of the conditions are true then we can skip reloading file metadata. The above situations never change file metadata, only table metadata is changed. http://gerrit.cloudera.org:8080/#/c/19838/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3942 PS2, Line 3942: equals(afterTabl > I am not 100% sure that we can ignore the case. I can imagine a properties Ack -- 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: 3 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, 19 May 2023 00:07:58 +0000 Gerrit-HasComments: Yes
