Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11193 )
Change subject: IMPALA-7425: Change incremental stats to pull from catalogd. ...................................................................... Patch Set 12: (9 comments) Patch lgtm pending some nits and one clarification on locking in CatalogServiceCatalog. http://gerrit.cloudera.org:8080/#/c/11193/12/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11193/12/be/src/common/global-flags.cc@214 PS12, Line 214: " nit: formatting? http://gerrit.cloudera.org:8080/#/c/11193/12/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/11193/12/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@637 PS12, Line 637: Preconditions.checkNotNull(partitions); nit: Preconditions.checkState(BackendConfig.INSTANCE.pullIncrementalStatistics() || test)? http://gerrit.cloudera.org:8080/#/c/11193/12/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@639 PS12, Line 639: try { Should we add any instrumentation for this block (or) expose this in the runtime profile as a metric somewhere? (can help us in benchmarking too). Add a TODO if you plan on doing this later? http://gerrit.cloudera.org:8080/#/c/11193/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11193/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@396 PS12, Line 396: TTableName tableName = request.table_name; Preconditions.checkState(configEnabled || test) http://gerrit.cloudera.org:8080/#/c/11193/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@419 PS12, Line 419: Do we need to acquire a table read lock to disallow concurrent modifications? http://gerrit.cloudera.org:8080/#/c/11193/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@421 PS12, Line 421: Preconditions.checkState(table.isLoaded()); Is this still needed after checks in L409/416? http://gerrit.cloudera.org:8080/#/c/11193/12/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/11193/12/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@685 PS12, Line 685: public TAccessLevel getAccessLevel() { : return accessLevel_; : } undo? http://gerrit.cloudera.org:8080/#/c/11193/12/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/11193/12/fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java@60 PS12, Line 60: public static TPartitionStats partStatsFromCompressedBytes( : byte[] compressedStats, FeFsPartition part) : throws ImpalaException { fit in 2 lines? http://gerrit.cloudera.org:8080/#/c/11193/12/fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java@69 PS12, Line 69: part.getPartitionName() Is 'part' really needed here? Can be handled at the caller? -- To view, visit http://gerrit.cloudera.org:8080/11193 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d564808ca5157afe4e091909ca6cdac76e60d6e Gerrit-Change-Number: 11193 Gerrit-PatchSet: 12 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: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 05 Sep 2018 05:11:33 +0000 Gerrit-HasComments: Yes
