Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14039 )
Change subject: IMPALA-8818: Replace deque with spillable queue in BufferedPRS ...................................................................... Patch Set 10: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/14039/8/be/src/runtime/spillable-row-batch-queue.cc File be/src/runtime/spillable-row-batch-queue.cc: http://gerrit.cloudera.org:8080/#/c/14039/8/be/src/runtime/spillable-row-batch-queue.cc@80 PS8, Line 80: RETURN_IF_ERROR(state_->StartSpilling(mem_tracker_)); > The Buffer Pool already seems to include some metrics to indicate that spil That's a fair point. Most spilling operators have some single counter or ExecOption string with the word "Spilled" in it. I *think* this is useful for diagnosis to quickly see if something spilled without digging into BufferPool counters, but in some ways it would be better to have the BufferPOol counters that are consistent across operators. So yeah, I could go either way, ok to leave it out. We also have some grepping in the stress test to identify queries that spilled based on specific strings appearing: https://github.com/apache/impala/blob/master/tests/stress/query_runner.py#L69 http://gerrit.cloudera.org:8080/#/c/14039/8/fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java File fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java: http://gerrit.cloudera.org:8080/#/c/14039/8/fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java@78 PS8, Line 78: Preconditions.checkState(memEstimateBytes_ >= 0, "Mem estimate must be set"); > I added that check to the ResourceProfile constructor, should I add it here Oh I missed that. Including it in the constructor seems sufficient. -- To view, visit http://gerrit.cloudera.org:8080/14039 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10f9e72374cdf9501c0e5e2c5b39c13688ae65a9 Gerrit-Change-Number: 14039 Gerrit-PatchSet: 10 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 22 Aug 2019 06:12:50 +0000 Gerrit-HasComments: Yes
