Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11388 )
Change subject: WIP IMPALA-7527: add fetch-from-catalogd cache info to profile ...................................................................... Patch Set 5: (4 comments) added e2e test, restricted nested scopes, added rpc metrics (per query, not broken down by rpc method) http://gerrit.cloudera.org:8080/#/c/11388/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/11388/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@309 PS3, Line 309: throw new TException(e); > Should we track num rpcs? Done. These stats add the following lines (for a miss): ... - CatalogFetch.RPCs.Requests: 4 (4) - CatalogFetch.RPCs.Time: 337ms - CatalogFetch.RPCs.Bytes: 33.28 KB (34082) ... http://gerrit.cloudera.org:8080/#/c/11388/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@349 PS3, Line 349: return (ValueType)cache_.get(key, new Callable<ValueType>() { > Does it help to implement a ScopedTimerWithStats (try-with-resources way)? it might, but for three call-sites, I didn't think it was worth it right now. http://gerrit.cloudera.org:8080/#/c/11388/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@363 PS3, Line 363: temString, hit.getRef() ? "hit" : "miss"); : } > this is kinda weird. Maybe overload this method that takes a single bool wh its the only call-site with this 1 | 0 logic. the rest do not use it, so I don't think its worth it to generalize it at this point. http://gerrit.cloudera.org:8080/#/c/11388/3/fe/src/main/java/org/apache/impala/service/FrontendProfile.java File fe/src/main/java/org/apache/impala/service/FrontendProfile.java: http://gerrit.cloudera.org:8080/#/c/11388/3/fe/src/main/java/org/apache/impala/service/FrontendProfile.java@151 PS3, Line 151: oldThreadLocalValue_ = THREAD_LOCAL.get(); > k I think I'll just prevent nested scopes and document it as such added the precondition and a todo to enable nested scopes. -- To view, visit http://gerrit.cloudera.org:8080/11388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I808d55a225338912ebaebd0cf71c36db944b7276 Gerrit-Change-Number: 11388 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 18 Sep 2018 16:05:56 +0000 Gerrit-HasComments: Yes
