Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11511 )
Change subject: IMPALA-7531: Daemon level catalog cache metrics ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/11511/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/11511/1/be/src/service/impala-server.cc@1212 PS1, Line 1212: e_miss_count > if there's a miss, there's a load? just wondering it they're redundant. I s Yea, this is interesting. I'm pretty sure that this related to the way we are loading partitions. I ran a bunch of other queries and the hit/load was 1:1 and then I loaded a partitioned table and it diverged. We load partitions differently from other metadata. We make an initial pass which checks if the cache has given partitions (hits/misses recorded) and then we bulk load the missing ones and put them into cache one at a time. It is not super clear to me how Guava accounts for this. Not super sure if puts() this way are incremented for loadSuccess. https://github.com/google/guava/blob/v14.0/guava/src/com/google/common/cache/LocalCache.java#L2885 I can dig into it, but will keep it as is for now. http://gerrit.cloudera.org:8080/#/c/11511/1/be/src/util/impalad-metrics.h File be/src/util/impalad-metrics.h: http://gerrit.cloudera.org:8080/#/c/11511/1/be/src/util/impalad-metrics.h@134 PS1, Line 134: /// Ratio of Impalad Catalog cache requests that were hits. > Is this since the beginning of time? Yes, clarified. http://gerrit.cloudera.org:8080/#/c/11511/1/be/src/util/impalad-metrics.cc File be/src/util/impalad-metrics.cc: http://gerrit.cloudera.org:8080/#/c/11511/1/be/src/util/impalad-metrics.cc@211 PS1, Line 211: catalog_metrics->AddDoubleGauge(ImpaladMetricKeys::CATALOG_CACHE_AVG_LOAD_TIME, 0); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/11511/1/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/11511/1/common/thrift/metrics.json@1697 PS1, Line 1697: "key": "catalog.cache.average-load-time" > I think it's super confusing that a metric for Impalad starts with "catalog Doesn't the context help here? We have been doing it like this for a few other similar metrics, say catalog.ready, catalog.curr-version, catalog.num-tables etc... I'm afraid changing all of them might break some monitoring tools? http://gerrit.cloudera.org:8080/#/c/11511/1/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java File fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java: http://gerrit.cloudera.org:8080/#/c/11511/1/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@370 PS1, Line 370: if (!(provider instanceof CatalogdMetaProvider)) return; > Not now, but there is another one called DirectMetaProvider that's easy to Yep, I added this check for DirectMetaProvider. http://gerrit.cloudera.org:8080/#/c/11511/1/tests/custom_cluster/test_local_catalog.py File tests/custom_cluster/test_local_catalog.py: http://gerrit.cloudera.org:8080/#/c/11511/1/tests/custom_cluster/test_local_catalog.py@35 PS1, Line 35: def get_debug_page(self, page_url): : """Returns the content of the debug page 'page_url' as json.""" : response = requests.get(page_url) : assert response.status_code == requests.codes.ok : return json.loads(response.text) > use get_debug_webpage_json from impala_service.py Done http://gerrit.cloudera.org:8080/#/c/11511/1/tests/custom_cluster/test_local_catalog.py@50 PS1, Line 50: \ > flake8: E502 the backslash is redundant between brackets Done -- To view, visit http://gerrit.cloudera.org:8080/11511 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I23c131b77ca84aa4df8919213bbd83944fa112a5 Gerrit-Change-Number: 11511 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 25 Sep 2018 22:58:56 +0000 Gerrit-HasComments: Yes
