Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11534 )
Change subject: IMPALA-7622: adds profile metrics when fetching incremental stats ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/11534/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java File fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java: http://gerrit.cloudera.org:8080/#/c/11534/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@651 PS1, Line 651: Stopwatch sw = new Stopwatch().start(); Should we break it down further into RPC fetch time and decompression time? http://gerrit.cloudera.org:8080/#/c/11534/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@675 PS1, Line 675: numPartitionsReceived 'received' seems like an internal terminology due to out of sync coordinator and catalog. May be just say "totalPartitions" or something? totalParts = partitions.size() (instead of incrementing inside the loop) http://gerrit.cloudera.org:8080/#/c/11534/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@683 PS1, Line 683: partStatsFromCompressedBytes Also include decompressed bytes size from this method? StatsFetch.compressedByteSize StatsFetch.decompressedByteSize http://gerrit.cloudera.org:8080/#/c/11534/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@696 PS1, Line 696: recordFetchMetrics(numBytes, numPartitionsReceived, numPartitionsWithStats, sw); nit: should we skip if an exception was thrown? http://gerrit.cloudera.org:8080/#/c/11534/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@700 PS1, Line 700: private static void recordFetchMetrics(int numBytes, int numPartitionsReceived, method doc. http://gerrit.cloudera.org:8080/#/c/11534/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@704 PS1, Line 704: NONE BYTES for pretty printing? http://gerrit.cloudera.org:8080/#/c/11534/1/tests/common/custom_cluster_test_suite.py File tests/common/custom_cluster_test_suite.py: http://gerrit.cloudera.org:8080/#/c/11534/1/tests/common/custom_cluster_test_suite.py@190 PS1, Line 190: i ouch http://gerrit.cloudera.org:8080/#/c/11534/1/tests/custom_cluster/test_pull_stats.py File tests/custom_cluster/test_pull_stats.py: http://gerrit.cloudera.org:8080/#/c/11534/1/tests/custom_cluster/test_pull_stats.py@64 PS1, Line 64: # and should have been fetched. Add another partition and run incremental stats again and assert sure totalParts and NumPartitionsWithStats counts? -- To view, visit http://gerrit.cloudera.org:8080/11534 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic9b268548c7a98c751eb99855ee08313d1d5a903 Gerrit-Change-Number: 11534 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Comment-Date: Thu, 27 Sep 2018 17:07:11 +0000 Gerrit-HasComments: Yes
