Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17576 )
Change subject: IMPALA-10746: Drop table/db from catalog cache when drop table/db HMS apis are accessed from catalog's metastore server. ...................................................................... Patch Set 13: (5 comments) http://gerrit.cloudera.org:8080/#/c/17576/10/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/17576/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2515 PS10, Line 2515: Table existingTbl = db.getTable(tblName); : if (existingTbl == null) return null; > It is. We need existingTbl to get eventId which is set in newly initialized Oh I see. I missed that part. Thanks for clarifying. http://gerrit.cloudera.org:8080/#/c/17576/10/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17576/10/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@462 PS10, Line 462: : // Parse db name. Throw error if parsing fails. : String dbName; : String catName; : String[] catAndDbName = : > Good catch! I agree with the point 2 above. But I think the deleteEventLog should be added from this method not from within removeDbIfnotAddedLater and removeTableIfNotAddedLater since they are used from events processor as well. http://gerrit.cloudera.org:8080/#/c/17576/13/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17576/13/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@479 PS13, Line 479: .getNextMetastoreEvents(catalog_, currentEventId, > @Vihang: Filter for drop_database in catalogOpExecutor is I believe it fetches for DROP_TABLE events as well because a cascade drop operation on db will drop the tables as well and we would need to add them to the deleteEventLog. I don't think this API drop_database supports cascade (although I didn't verify in the code). If this code would be executed in a drop database cascade operation then we should fetch the drop_table events as well. http://gerrit.cloudera.org:8080/#/c/17576/13/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@456 PS13, Line 456: if (!invalidateCacheOnDDLs_) { : LOG.debug("invalidateCacheOnDDLs_ flag is false, skipping " + : "cache update for hms operation drop_database on db: " + : databaseName); : return; : } : : // Parse db name. Throw error if parsing fails. : String dbName; : String catName; : String[] catAndDbName = : MetaStoreUtils.parseDbName(databaseName, serverConf_); : catName = catAndDbName[0]; : dbName = catAndDbName[1]; : // get DB to drop from catalog cache : Db dbToDrop = catalog_.getDb(dbName); : if(dbToDrop == null) { : // dbToDrop doesn't exist in cache : return; : } : List<NotificationEvent> events; : try { : events = MetastoreEventsProcessor : .getNextMetastoreEvents(catalog_, currentEventId, : event -> event.getEventType() : .equalsIgnoreCase(MetastoreEvents.DropDatabaseEvent : .DROP_DATABASE_EVENT_TYPE) : && catName.equalsIgnoreCase(event.getCatName()) : && dbName.equalsIgnoreCase(event.getDbName())); : } catch (ImpalaException e) { : String errorMsg = "Unable to process Drop database event for db: " + : dbToDrop.getName(); : LOG.error(errorMsg, e); : throw new MetaException(errorMsg); : } : if (events.isEmpty()) { : throw new MetaException( : "Drop database event not received. Check if notification " + : "events are configured in hive metastore"); : } : long dropEventId = events.get(events.size() - 1).getEventId(); : boolean isRemoved = : catalogOpExecutor_.removeDbIfNotAddedLater(dropEventId, : dbToDrop.getMetaStoreDb()); : if (isRemoved) { : LOG.info("Removed database: " + databaseName + : " from cache due to HMS API: drop_database"); > I intentionally didn't do it since there is only one drop db api. Let me kn I felt the code readability improves if you refactor it out. But I don't have strong opinion about it. http://gerrit.cloudera.org:8080/#/c/17576/13/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@2968 PS13, Line 2968: Map<String, String> tblProperties = catalogTbl.getMetaStoreTable().getParameters(); : if (tblProperties == null || MetaStoreUtils.isTransactionalTable(tblProperties)) { > How would it help? If we use Impala's AcidUtils method the logic would be: I agree that functionally they both for same. My point was more related to consistency within the code. Several other places in the catalogd use AcidUtils.isTransactionalTable. It makes it easier to lookup the places as well easy to change/enhance the implementation if needed. The other advantage is that we don't need to depend on a non-public API in HMS which can be changed/removed without any notice. -- To view, visit http://gerrit.cloudera.org:8080/17576 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2e2ad2630e2028b8ad26a6272ee766b27e0935c Gerrit-Change-Number: 17576 Gerrit-PatchSet: 13 Gerrit-Owner: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Yu-Wen Lai <[email protected]> Gerrit-Comment-Date: Mon, 02 Aug 2021 20:43:37 +0000 Gerrit-HasComments: Yes
