Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12591 )
Change subject: IMPALA-7972 Detect self-events to avoid unnecessary invalidates ...................................................................... Patch Set 5: (16 comments) Mostly general comments. I think the code in CatalogOpExecutor is in dire need of some refactoring (outside the scope of this patch). We should probably factor out all the common stuff into CatalogOpCtx and then pass it around instead of adding new parameters for every method. http://gerrit.cloudera.org:8080/#/c/12591/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12591/5//COMMIT_MSG@19 PS5, Line 19: and the catalog version number. The uuid is generated for each catalogservice when nit: line overflow http://gerrit.cloudera.org:8080/#/c/12591/5/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/12591/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@189 PS5, Line 189: // unique identifier of this catalog service : private static final String SERVICE_UUID = UUID.randomUUID().toString(); Why not use the catalogServiceId_ below? Use TUniqueIdUtil#printId() to convert it to a string. http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@741 PS5, Line 741: if (!isExternalEventProcessingEnabled()) return result; Isn't this a preconditions check? http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@785 PS5, Line 785: ersion version http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@805 PS5, Line 805: tbl.addToVersionsForInflightEvents(versionNumber); Looks like this can silently fail. How about logging something in that case? http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/Table.java@20 PS5, Line 20: import java.util.ArrayDeque; unused? http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/Table.java@24 PS5, Line 24: import java.util.Deque; unused? http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/Table.java@117 PS5, Line 117: private static final int MAX_NUMBER_OF_INFLIGHT_EVENTS = 10; doc http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/Table.java@120 PS5, Line 120: in seconds to initial ? http://gerrit.cloudera.org:8080/#/c/12591/5/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/12591/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@889 PS5, Line 889: protected List<Long> pendingVersionNumbersFromCatalog_ = Collections.EMPTY_LIST; Add a doc? I think this crucial to the logic here. http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@900 PS5, Line 900: * This method detects if this event is self-generated or not. In order to May be add a pointer to the MetastoreEvents class where you defined what a self-event is? http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1147 PS5, Line 1147: /** : * Adds self-event identifiers in the table parameters : */ : public static void addCatalogServiceIdentifiers( : org.apache.hadoop.hive.metastore.api.Table msTbl, long catalogVersionId) { : msTbl.putToParameters( : CATALOG_SERVICE_ID_PROP_KEY, CatalogServiceCatalog.getServiceUUID()); : msTbl.putToParameters(CATALOG_VERSION_PROP_KEY, String.valueOf(catalogVersionId)); : } : : /** : * Adds self-event identifiers in the partition parameters : */ : public static void addCatalogServiceIdentifiers( : org.apache.hadoop.hive.metastore.api.Partition partition, long catalogVersionId) { : partition.putToParameters( : CATALOG_SERVICE_ID_PROP_KEY, CatalogServiceCatalog.getServiceUUID()); : partition.putToParameters(CATALOG_VERSION_PROP_KEY, String.valueOf(catalogVersionId)); : } Should these be no-ops if the event processing is not enabled? http://gerrit.cloudera.org:8080/#/c/12591/5/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/12591/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@397 PS5, Line 397: updateStatus(EventProcessorStatus.ERROR); Probably needs rebase here. http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@521 PS5, Line 521: long newCatalogVersion = catalog_.incrementAndGetCatalogVersion(); : boolean reloadMetadata = true; Move this above pass newCatalogVersion to alterTableOrRename() rather than doing currVersion + 1. Thats easier to reason about. http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2285 PS5, Line 2285: * catalog cache and the HMS. Add something about newCatalogVersion and how we use it? http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2561 PS5, Line 2561: long newCatalogVersion = catalog_.getCatalogVersion() + 1; Please refer to my comment above. -- To view, visit http://gerrit.cloudera.org:8080/12591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6db0d7f7fe465158fc8cb9d6b6b57a321827b353 Gerrit-Change-Number: 12591 Gerrit-PatchSet: 5 Gerrit-Owner: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Bharath Krishna <bhar...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Thu, 28 Feb 2019 03:52:26 +0000 Gerrit-HasComments: Yes
