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 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/19838/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19838/1//COMMIT_MSG@19 PS1, Line 19: file_metadata_reload_propertie > nit: can we use a name indicating this is a list of table properties, e.g. Ack http://gerrit.cloudera.org:8080/#/c/19838/1//COMMIT_MSG@25 PS1, Line 25: statements the file metadata isn't reloaded. > Could you add a custom cluster test for the new flag? Currently, there is no end-to-end test that verifies whether file metadata is reloaded or not. Can you suggest how to achieve this? http://gerrit.cloudera.org:8080/#/c/19838/1/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/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2569 PS1, Line 2569: > nit: please also comment this with /*partitionToEventId*/ Ack http://gerrit.cloudera.org:8080/#/c/19838/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3881 PS1, Line 3881: } > nit: event Done http://gerrit.cloudera.org:8080/#/c/19838/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3894 PS1, Line 3894: nt. : */ > Shouldn't we check these two before checking the whitelist table properties I would like to disable the optimization by setting the above flag to empty in case something works unexcepted. What do you think? I can update the description of the config flag to explain the same. http://gerrit.cloudera.org:8080/#/c/19838/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3897 PS1, Line 3897: org.apache.hadoop.hive.metastore.api.Table beforeTable, > Should we check if there are other changes between "beforeTable" and "after AFAIK, you can only change one specific thing (either serde or location or tbl properties etc.) on an alter event. For location change, we'll have to reload file metadata, which we are anyway doing now since we'll be returning false in that condition. http://gerrit.cloudera.org:8080/#/c/19838/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3907 PS1, Line 3907: return true; : } : return false; > nit: needs a space after "//". Needs brackets for the if-clause based on ou Done http://gerrit.cloudera.org:8080/#/c/19838/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3912 PS1, Line 3912: private boolean isFieldSchemaChanged( > nit: need spaces Done http://gerrit.cloudera.org:8080/#/c/19838/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3928 PS1, Line 3928: > Please make 'whitelistConfigs' a constant field so we don't need to parse t Ack http://gerrit.cloudera.org:8080/#/c/19838/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3932 PS1, Line 3932: org.apache.hadoop.hive.metastore.api.Table beforeTable, > Are we missing the case of "before && !after", i.e. the property is removed Ack http://gerrit.cloudera.org:8080/#/c/19838/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3944 PS1, Line 3944: } > nit: remove blank lines 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: 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: Tue, 09 May 2023 00:52:20 +0000 Gerrit-HasComments: Yes
