Vuk Ercegovac 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) 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? Done 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.pullIncrementalStatist Done 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 ru added a todo to the caller before, so brought it down to this method to make it clearer. 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) Done 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 modification yes, good catch. 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? The check on L409 is insufficient. A table can be Incomplete, but not yet loaded (apparently, its determined by the presence of a "cause"). The check on L416 implies that the table is loaded since HdfsTable does not override the isLoaded method, which is true by default. Both examples require a little digging, so I prefer to state what I expect here. With the inheritance chain on Table, etc, default behavior can be inadvertently changed. 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? Done 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? Done 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? I was tempted to handle it at the caller, but this is for the null case, not an exception. If we don't want to replace null here with an exception, then yes, makes sense to handle it 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 06:30:47 +0000 Gerrit-HasComments: Yes
