Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12549 )
Change subject: IMPALA-7975 : Improve supportability of the automatic invalidate feature ...................................................................... Patch Set 2: (15 comments) Haven't looked at each metric in detail but have some high-level comments on the code flow. Will take a deeper look in the next pass. http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/catalog/catalog-server.h File be/src/catalog/catalog-server.h: http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/catalog/catalog-server.h@212 PS2, Line 212: Method doc http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/catalog/catalog-server.cc@399 PS2, Line 399: TGetEventProcessorMetricsResponse eventProcessorMetricsResponse; : status = catalog_->GetEventProcessorMetrics(&eventProcessorMetricsResponse); : if (!status.ok()) { : LOG(ERROR) << "Error refreshing metastore event metrics: " << status.GetDetail(); : continue; : } merge this with the above call? Avoids an unncessary JNI call. http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/catalog/catalog-server.cc@521 PS2, Line 521: url handler for the metastore events page. It calls into JniCatalog to get the latest : // metastore event processor metrics and adds it to the document Move to header. http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/catalog/catalog.h File be/src/catalog/catalog.h: http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/catalog/catalog.h@112 PS2, Line 112: /// Gets all functions in the catalog matching the parameters in the given newline and method doc. http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/util/event-metrics.h File be/src/util/event-metrics.h: http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/util/event-metrics.h@22 PS2, Line 22: // class which is used to fetch the metastore event related metrics from catalog /// http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/util/event-metrics.h@28 PS2, Line 28: static void refresh(TGetEventProcessorMetricsResponse* response); /// http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/util/event-metrics.h@30 PS2, Line 30: static IntCounter* NUM_EVENTS_RECEIVED_COUNTER; Add inline comments for each of these? What they represent? http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/util/event-metrics.cc File be/src/util/event-metrics.cc: http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/util/event-metrics.cc@56 PS2, Line 56: MetricGroup* event_metrics = metric_group->GetOrCreateChildGroup("events"); If event processing is disabled, do we need to register them? http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/util/event-metrics.cc@76 PS2, Line 76: // refresh all the metrics which are used to display on webui based on the given response Move to header http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/util/event-metrics.cc@82 PS2, Line 82: DCHECK(response->__isset.events_process_duration_mean); if you are DCHECKing here why the if(__isset)s below? http://gerrit.cloudera.org:8080/#/c/12549/2/be/src/util/event-metrics.cc@113 PS2, Line 113: LOG(ERROR) << "Received a null response when trying to refresh metastore event metrics"; Move to the top? if (!response) LOG.error() return; http://gerrit.cloudera.org:8080/#/c/12549/2/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/12549/2/common/thrift/JniCatalog.thrift@759 PS2, Line 759: // status of event processor Add new lines between each. http://gerrit.cloudera.org:8080/#/c/12549/2/common/thrift/JniCatalog.thrift@775 PS2, Line 775: // Summary of the event processor metrics which include status and more detailed metrics It looks like there are a couple of ways this is used. One to get metrics 1-9, and two the summary. Can we split those up into two different groups and merge the first group into TGetCatalogServerMetricsResponse? That way in RefreshMetrics() we have just 1 JNI call and for the web end point, we could call the latter. That way we don't build the summary string and discard it every 2s. http://gerrit.cloudera.org:8080/#/c/12549/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/12549/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@232 PS2, Line 232: protected final Metrics metrics_; Curious why this would be in the Event class? I'd guess it should be something more global accessible by all events? http://gerrit.cloudera.org:8080/#/c/12549/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/12549/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@378 PS2, Line 378: metrics_ This is thread safe right? -- To view, visit http://gerrit.cloudera.org:8080/12549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I23cb3aa866879eca03c64ab881796eaa9caa0337 Gerrit-Change-Number: 12549 Gerrit-PatchSet: 2 Gerrit-Owner: Vihang Karajgaonkar <vih...@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-Comment-Date: Fri, 22 Feb 2019 00:11:38 +0000 Gerrit-HasComments: Yes
