Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11341 )
Change subject: IMPALA-7424: Reduce in-memory footprint of incremental stats ...................................................................... Patch Set 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/11341/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11341/5//COMMIT_MSG@14 PS5, Line 14: gzipped gzip is not present in this change, so either state 'deflate-compressed' or just 'compressed'. http://gerrit.cloudera.org:8080/#/c/11341/5/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/11341/5/common/thrift/CatalogService.thrift@302 PS5, Line 302: // Set if 'want_partition_stats' was set in TTableInfoSelector. and the partition has associated stats. just to disambiguate the case where stats are not present-- is this set empty or unset? http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java File fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java: http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java@160 PS5, Line 160: filtered I think you're using this term since the prev. method was called getFilteredHmsParameters. simpler to just say that the keys used to store TPartitionStats are not returned. http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@524 PS5, Line 524: // when the partition is loaded. and when its updated (from CatalogOpExecutor, through the Util to serialize, and back to setPartitionStatsBytes) http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java File fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java: http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java@76 PS5, Line 76: nit: add the hyphen http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java@109 PS5, Line 109: // null move this to the method javadoc. also mention the deletion on L122 http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java@123 PS5, Line 123: deletePartStats(partition); this was somewhat surprising at first... was wondering why the deletion if TPartStats is coming in from the caller. makes more sense to me when I see that its an overwrite and we're making TPartStats the master copy. I think just comments will clarify. http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java@128 PS5, Line 128: Error saving partition stats: this is redundant with the prefix in L126 http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java@129 PS5, Line 129: rror saving partitio same here http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/util/CompressionUtil.java File fe/src/main/java/org/apache/impala/util/CompressionUtil.java: http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/util/CompressionUtil.java@40 PS5, Line 40: ByteArrayOutputStream bos = new ByteArrayOutputStream(input.length); precondition that input is not-null or return null if that's the case. same for the other method. http://gerrit.cloudera.org:8080/#/c/11341/5/fe/src/main/java/org/apache/impala/util/CompressionUtil.java@59 PS5, Line 59: IOUtils.copy(new InflaterInputStream(new ByteArrayInputStream(input)), out); make these more symmetric. the other one returns in the try-block, this one doesn't. can make it easier to see that a null is returned for similar cases. -- To view, visit http://gerrit.cloudera.org:8080/11341 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39f02ebfa0c6e9b0baedd0d76058a1b34efb5a02 Gerrit-Change-Number: 11341 Gerrit-PatchSet: 5 Gerrit-Owner: Bharath Vissapragada <[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: Thu, 30 Aug 2018 17:50:36 +0000 Gerrit-HasComments: Yes
