Quanlong Huang 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 11: (6 comments) LGTM. Just have some suggestions in the logging since we already have logs on the full table name for events. It'd be nice to add some more details about the ALTER_TABLE changes so admins don't need to check Hive's NOTIFICATION_LOGS. http://gerrit.cloudera.org:8080/#/c/19838/11/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/19838/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1469 PS11, Line 1469: if (canBeSkipped()) { This also checks tblproperties but acts like blacklist (i.e. skip if match). Should we keep or remove it? http://gerrit.cloudera.org:8080/#/c/19838/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1537 PS11, Line 1537: infoLog("Change in number of columns detected for table {} in the database {} ", nit: let's try to log the numbers. Also use fully qualified table name to ease grep commands. E.g. Change in number of columns detected for table {}.{} from {} to {} http://gerrit.cloudera.org:8080/#/c/19838/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1546 PS11, Line 1546: infoLog("Change in table schema detected for table {} in the database {} ", nit: let's try to log the new and old name and type of this column http://gerrit.cloudera.org:8080/#/c/19838/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1555 PS11, Line 1555: infoLog("Change in Ownership detected for table {} in the database {} ", tblName_, nit: let's try to log the new and old owners, e.g. Change in Ownership detected. Table: {}.{}, oldOwner: {}, newOwner: {} http://gerrit.cloudera.org:8080/#/c/19838/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1568 PS11, Line 1568: infoLog("Change in whitelisted table properties detected for table {} in the" + nit: let's log the new and old strings http://gerrit.cloudera.org:8080/#/c/19838/11/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/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2813 PS11, Line 2813: alterTableSetOwnerFromImpala Shouldn't we do this in Hive? -- 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: 11 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-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 14 Jun 2023 01:46:29 +0000 Gerrit-HasComments: Yes
