Vuk Ercegovac 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? I don't think that level of detail is needed right now. 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 coordinato done. changed the metric name to TotalPartitions as well. 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? clarified to compressedByteSize. for decompressed, that size is inconvenient to get at the moment (its hidden in that util) and don't we really want the size of the deserialized object, not just the bytes? 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? my thought here was that time/effort was spent in that rpc so lets report what we have. 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. Done 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? Done 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 Done 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 total this functionality is tested in test_pull_stats. but added a test for this case for the sake of capturing it in the profile. -- 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-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 27 Sep 2018 19:06:21 +0000 Gerrit-HasComments: Yes
