Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22571 )
Change subject: IMPALA-13684: Improve waitForHmsEvent() to only wait for related events ...................................................................... Patch Set 5: (11 comments) Thanks for the review! Addressed some comments first. Will work on the remaining comments and add more test coverage. http://gerrit.cloudera.org:8080/#/c/22571/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22571/3//COMMIT_MSG@18 PS3, Line 18: > Do GRANT/REVOKE events also need to be checked? For example, "grant select These are executed in Ranger side and they won't generate HMS events. Instead, the Ranger plugin in Impala will poll updates from Ranger Admin Server every 30s (configured by ranger.plugin.hive.policy.pollIntervalMs in ranger-hive-security.xml). Admin users can also run a REFRESH AUTHORIZATION statement to make it synced immediately. FWIW, here are all the HMS event types: https://github.com/apache/hive/blob/183f8cb41d3dbed961ffd27999876468ff06690c/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/messaging/EventMessage.java#L33-L77 http://gerrit.cloudera.org:8080/#/c/22571/3//COMMIT_MSG@32 PS3, Line 32: until they are processed. To be safe, wait for all transactional > In this case, will the ALTER/DROP database events also be checked in case a The db events are checked in step 2 regardless whether the db is missing in catalog. I should update step 2 about this. BTW, "drop database cascade" will generate DROP_TABLE events for all the underlying tables before the DROP_DATABASE event. So we are pretty safe. http://gerrit.cloudera.org:8080/#/c/22571/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/22571/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@1112 PS3, Line 1112: eventRequest.setTableNames(metaDataFilter.getTableNames()); > Looks like NotificationRequest handles null tableNames arg and you don't ne I need to double check the behavior of empty list. http://gerrit.cloudera.org:8080/#/c/22571/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/22571/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1506 PS3, Line 1506: public static final List<String> TABLE_LIST_EVENT_TYPES = Lists.newArrayList( > Can these be declared final? Yeah, they should be final. http://gerrit.cloudera.org:8080/#/c/22571/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1635 PS3, Line 1635: */ > Change Found to Find Done http://gerrit.cloudera.org:8080/#/c/22571/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1649 PS3, Line 1649: for (TCatalogObject catalogObject: req.getObject_descs()) { > declare final. Right! This should be a Set. For declaring a local variable as final, I think it only helps the readability of the code in long methods, and avoid future developers unintentionally modifies the variable if this introduces bugs. This method is short (ignoring the comments). I think the compiler should be able to do its work well, e.g. detecting this variable won't be reassigned and performing the optimizations (if there are). http://gerrit.cloudera.org:8080/#/c/22571/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1690 PS3, Line 1690: NotificationFilter filter = e -> dbName.equalsIgnoreCase(e.getDbName()) > declare final Replied above. http://gerrit.cloudera.org:8080/#/c/22571/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1692 PS3, Line 1692: && eventTypes.contains(e.getEventType()); > Can filter be declared final static? Java does not support static local variables like C/C++ does. http://gerrit.cloudera.org:8080/#/c/22571/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1772 PS3, Line 1772: && tblNames.contains(e.getTableName().toLowerCase()) > declare final Replied above. http://gerrit.cloudera.org:8080/#/c/22571/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1773 PS3, Line 1773: && MetastoreShim.isDefaultCatalog(e.getCatName()) > Can filter be declared final static? Replied above. http://gerrit.cloudera.org:8080/#/c/22571/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1801 PS3, Line 1801: List<NotificationEvent> dbEvents = getNextMetastoreEventsInBatches( > Can filter be declared final static? Replied above. -- To view, visit http://gerrit.cloudera.org:8080/22571 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic033b7e197cd19505653c3ff80c4857cc474bcfc Gerrit-Change-Number: 22571 Gerrit-PatchSet: 5 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.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: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Tue, 18 Mar 2025 14:01:09 +0000 Gerrit-HasComments: Yes
