Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11388 )
Change subject: IMPALA-7527: add fetch-from-catalogd cache info to profile ...................................................................... Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/11388/6/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/11388/6/be/src/service/client-request-state.h@68 PS6, Line 68: > Add a method doc about who sets it etc. Done http://gerrit.cloudera.org:8080/#/c/11388/6/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/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@318 PS6, Line 318: CatalogFetch > Make these stuff static? Used below too. Done http://gerrit.cloudera.org:8080/#/c/11388/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@319 PS6, Line 319: TUnit.UNIT > Switch to TUnit::NONE for types like this to not include x (x) stuff? Maybe Done. good suggestion to get rid of the parens. http://gerrit.cloudera.org:8080/#/c/11388/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@400 PS6, Line 400: private void addStatsToProfile(String statsCategory, int numHits, int numMisses, > method doc. Done http://gerrit.cloudera.org:8080/#/c/11388/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@406 PS6, Line 406: UNIT > same as above. Done http://gerrit.cloudera.org:8080/#/c/11388/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@410 PS6, Line 410: UNIT > same as above. Done http://gerrit.cloudera.org:8080/#/c/11388/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@413 PS6, Line 413: UNIT > same as above Done http://gerrit.cloudera.org:8080/#/c/11388/6/tests/custom_cluster/test_local_catalog.py File tests/custom_cluster/test_local_catalog.py: http://gerrit.cloudera.org:8080/#/c/11388/6/tests/custom_cluster/test_local_catalog.py@218 PS6, Line 218: def test_cache_profile_metrics(self): > Can we roll these checks into the above tests? Do we need a separate one? I'd prefer to keep the check for this separate/smaller/simpler. Are you looking for extra coverage? One possibility here is to wrap the "execute ..." methods and check that for v2 coordinators we see this profile. http://gerrit.cloudera.org:8080/#/c/11388/6/tests/custom_cluster/test_local_catalog.py@225 PS6, Line 225: ret = > unusued? used in the assert below -- 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: 6 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Sat, 22 Sep 2018 02:21:17 +0000 Gerrit-HasComments: Yes
