[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/17859 )
Change subject: IMPALA-10926: Sync db/table in catalog cache to latest HMS event id when performing DDL operations via catalog HMS endpoints ...................................................................... Patch Set 2: (19 comments) http://gerrit.cloudera.org:8080/#/c/17859/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/17859/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@223 PS2, Line 223: public static final long LOCK_RETRY_TIMEOUT_MS = 7200000; 7200000 seems to be too large. How did you pick this value ? http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2142 PS2, Line 2142: LOG.trace("table {} exits in cache, last synced id {}", tbl.getFullName(), Why do you have many .trace() instead of .debug() ? http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3654 PS2, Line 3654: tableLoadingMgr_.start(); Since its no longer final, can you check the code to make sure, we are not keeping its reference somewhere else ? http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@51 PS2, Line 51: private static final Logger LOG = LoggerFactory.getLogger(TableLoader.class); What is the difference between LoggerFactory vs Logger ? http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@164 PS2, Line 164: // write lock is not required since it is full table reload Can there be multiple threads trying to do full table reload at the same time ? Would a write lock be useful in that case ? http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java File fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java: http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java@159 PS2, Line 159: private CatalogServiceCatalog catalog_; Why are you removing "final" for CatalogServiceCatalog and CatalogOpExecutor ? http://gerrit.cloudera.org:8080/#/c/17859/2/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/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@266 PS2, Line 266: // TODO: Should we skip creating a new metastore events factory I think it all depends on whether we can toggle this flag, without restarting CatalogD or not. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@835 PS2, Line 835: protected boolean shouldSkipWhenSyncingToLatestEventId() throws CatalogException { Please write the Javadoc for this method. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@966 PS2, Line 966: protected boolean shouldSkipWhenSyncingToLatestEventId() { May be you can add default interface method which returns false in the abstract class ? http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1163 PS2, Line 1163: String oldDbName = tableBefore_.getDbName(); Please use the variables directly from tableBefore and tableAfter. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1504 PS2, Line 1504: @Override You can avoid most of these by having default implementation, which returns false. http://gerrit.cloudera.org:8080/#/c/17859/2/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/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@357 PS2, Line 357: 1. Should we stop after processing drop_table Please remove these TODOs or update them as per what is needed in future, before you commit the changes. Looks like some of them are discussions and not strictly TODOs. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@475 PS2, Line 475: // TODO: should we ignore case? I think it should become a utility method. Otherwise, everywhere we have to keep thinking whether it should be ignore case or not ! http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@899 PS2, Line 899: protected Metrics getMetrics() { Why this has to be changed to "protected", if its only for testing ? http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@110 PS2, Line 110: // protected final boolean invalidateCacheOnDDLs_; Please remove the variable, if you don't need it. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@117 PS2, Line 117: // TODO: Should we honor fallbacktoHMS in case we I don't think that would be a good idea. If you keep doing it for some reason, events can pile up. So, I prefer you rather fail so that we can detect the problem early. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@509 PS2, Line 509: org.apache.impala.catalog.Table tbl = getTableAndAcquireWriteLock(partition.getDbName(), > line too long (92 > 90) Hope you are using the Java template. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@607 PS2, Line 607: // TODO: Should we sync the table till latest event id if partitions list There is no harm in doing it. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@1332 PS2, Line 1332: boolean isRename = !dbname.equalsIgnoreCase(newTable.getDbName()) || Do you have to distinguish between database rename and table rename ? -- To view, visit http://gerrit.cloudera.org:8080/17859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36364e401911352c4666674eb98c8d61bbaae9b9 Gerrit-Change-Number: 17859 Gerrit-PatchSet: 2 Gerrit-Owner: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 22 Sep 2021 17:43:09 +0000 Gerrit-HasComments: Yes
