Bharath Vissapragada 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 3: (7 comments) Some high-level comments. http://gerrit.cloudera.org:8080/#/c/11388/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11388/3//COMMIT_MSG@26 PS3, Line 26: Can you add a sample output here. http://gerrit.cloudera.org:8080/#/c/11388/3//COMMIT_MSG@32 PS3, Line 32: breaking down by category? > still hoping to get some feedback on the above Yep, just looking at the code, I have a feeling it could be chatty and most users don't probably need that information. But if you can paste some sample profiles somewhere (especially with a mix of cache hits + misses, with/without partitions, multiple partial RPCs etc.) that'd be great. http://gerrit.cloudera.org:8080/#/c/11388/3//COMMIT_MSG@33 PS3, Line 33: - worth trying to include the byte sizes of metadata fetched? Could be useful from incremental stats / large partitioned tables. 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: ret = FeSupport.GetPartialCatalogObject(new TSerializer().serialize(req)); Should we track num rpcs? http://gerrit.cloudera.org:8080/#/c/11388/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@349 PS3, Line 349: Stopwatch sw = new Stopwatch().start(); Does it help to implement a ScopedTimerWithStats (try-with-resources way)? http://gerrit.cloudera.org:8080/#/c/11388/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@363 PS3, Line 363: /*numHits=*/hit.getRef() ? 1 : 0, : /*numMisses=*/hit.getRef() ? 0 : 1 this is kinda weird. Maybe overload this method that takes a single bool which calls the helper? 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(); > Just felt like it's good practice to treat scopes as nestable and "restore" Ya I think we should have some guard that prevents nested scopes. I doubt if anyone will use it, but still :-) -- 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: 3 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: Fri, 07 Sep 2018 05:27:20 +0000 Gerrit-HasComments: Yes
