Vihang Karajgaonkar 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 6: (15 comments) Added suggested changes by Bharath. Also, added metrics for number of self-events and number of times event processor successfully invalidated a table. 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 > nit: line overflow Done 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 > Why not use the catalogServiceId_ below? Use TUniqueIdUtil#printId() to con I didn't realize that we already have a service id. Removed this and using it instead. http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@741 PS5, Line 741: Preconditions.checkState(isExternalEventProcessingEnabl > Isn't this a preconditions check? Done http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@785 PS5, Line 785: > version Done http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@805 PS5, Line 805: String.format("Table %s not found", new Tabl > Looks like this can silently fail. How about logging something in that case Added a log.warn in addToVersionsForInflightEvents method. 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.ArrayList; > unused? Done 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.List; > unused? Done http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/Table.java@117 PS5, Line 117: // FIFO list of versions for all the in-flight metastore events in this table > doc Done http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/Table.java@120 PS5, Line 120: new LinkedList<>(); > ? Had changed the implementation and forgot to update the document later on. Fixed it now. 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 long versionNumberFromEvent_ = -1; > Add a doc? I think this crucial to the logic here. Done http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@900 PS5, Line 900: */ > May be add a pointer to the MetastoreEvents class where you defined what a Done. Also added more detail regarding the case when subsequent event with the same serviceid and version can show up http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1147 PS5, Line 1147: */ : private IgnoredEvent( : CatalogServiceCatalog catalog, Metrics metrics, NotificationEvent event) { : super(catalog, metrics, event); : } : : @Override : public void process() { : debugLog("Ignored"); : } : : @Override : protected boolean isEventProcessingDisabled() { : return false; : } : } : } : : > Should these be no-ops if the event processing is not enabled? These methods were moved to CatalogOpExecutor since we wanted to get the serviceId from CatalogServiceCatalog's serviceId now. Added a check to make them a no-op when event processing is disabled. 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: return; > Probably needs rebase here. Looks like the patch for IMPALA-8240 didn't merge yet so there is nothing to rebase yet. 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@2285 PS5, Line 2285: * catalog cache and the HMS. > Add something about newCatalogVersion and how we use it? Done http://gerrit.cloudera.org:8080/#/c/12591/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2561 PS5, Line 2561: msTbl.setTableName(newTableName.getTbl()); > Please refer to my comment above. Good suggestion. done. -- 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: 6 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 22:56:15 +0000 Gerrit-HasComments: Yes
