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 6: (4 comments) this round completes the fixes. next up is to replace the ids with fetching all. http://gerrit.cloudera.org:8080/#/c/11193/4/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/4/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@447 PS4, Line 447: Collection<? extends FeFsPartition> allPartitions = > I think we need more counters/timeline stuff around these RPCs in the incre will leave this as a todo for this change. timeline is not available here afaict, so need to see (or change) where this is convenient. http://gerrit.cloudera.org:8080/#/c/11193/4/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/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@406 PS4, Line 406: // Table could be null if it does not exist anymore. : if (table == null) return stats; > should we throw an exception in that case? Otherwise, if you ran COMPUTE ST good point, done. http://gerrit.cloudera.org:8080/#/c/11193/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@419 PS4, Line 419: + " loaded table: " + request.table_name); > maybe include the 'cause' of the IncompleteTable? Done http://gerrit.cloudera.org:8080/#/c/11193/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@422 PS4, Line 422: / Table is of the wrong type. : if (!(table instanceof FeFsTable)) { : throw new CatalogException("Partition statistics can only be requested for FS" : + " tables, type is: " + table.getClass().getCanonicalName()); : } > Can we make this a preconditions check? We do such checks in ComputeStatsSt actually, type-checker guarantees this from ComputStatsStmt -- 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: 6 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: Tue, 28 Aug 2018 16:52:09 +0000 Gerrit-HasComments: Yes
