Dan Hecht has posted comments on this change. Change subject: IMPALA-4674: Part 2: port backend exec to BufferPool ......................................................................
Patch Set 29: (26 comments) http://gerrit.cloudera.org:8080/#/c/5801/29/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: PS29, Line 805: if (failed_to_add) { : parent->CleanupHashTbl(agg_fn_evals, it); : hash_tbl->Close(); : hash_tbl.reset(); : aggregated_row_stream->Close(NULL, RowBatch::FlushMode::NO_FLUSH_RESOURCES); : RETURN_IF_ERROR(status); : return Status(TErrorCode::INTERNAL_ERROR, : Substitute("Unexpected error spilling aggregate node $0 to disk: should have " : "enough memory to append to stream. $1", : parent->id_, parent->buffer_pool_client_.DebugString())); : } not ready to turn that into just a dcheck? i'm okay with leaving it for now, though not sure how we can exercise that code. PS29, Line 1133: if (single_partition_idx != -1 && i != single_partition_idx) { : hash_partitions_.push_back(nullptr); : continue; : } I think this would read easier like this: if (single_partition_idx == -1 || i == single_partition_idx) { ... create partition ... } else { hash_partitions_.push_back(nullptr); } seems more obvious we just have two cases here and the positive conditional seems easier to think about. PS29, Line 1149: ; . PS29, Line 1152: success got_memory PS29, Line 1254: to build from the rows of the spilled partition not sure what that means PS29, Line 1308: Free up a buffer's worth of : // reservation the code looks like it potentially frees up more than a buffer worth. does this mean to say "Free up at least a buffer's worth"? Line 1396: id_, buffer_pool_client_.DebugString())); just curious: why do you expect a bug to trigger this more likely than a bug that would trigger the DCHECK(got_memory) cases? Line 1447: partition->unaggregated_row_stream->UnpinStream(BufferedTupleStreamV2::UNPIN_ALL); at this point, the streams are already unpinned right, and so we are just unpinning the iterator pages? or no? Line 1454: partition->Close(true); how about: if (partition != nullptr) partition->Close(true); Line 1468: if (partition == nullptr) continue; let's invert that since it's only one stmt in the block (even if the if-stmt doesn't fit on one line). http://gerrit.cloudera.org:8080/#/c/5801/29/be/src/exec/partitioned-aggregation-node.h File be/src/exec/partitioned-aggregation-node.h: PS29, Line 374: success got_memory? PS29, Line 676: blocks pages? PS29, Line 723: one nice http://gerrit.cloudera.org:8080/#/c/5801/29/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: PS29, Line 326: numeric_limits<int64_t>::max( alternatively, should that use the value passed as the third param to ParseMemSpec()? I agree this case is weird, not really sure the right thing. http://gerrit.cloudera.org:8080/#/c/5801/26/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS26, Line 149: bytes_limit = query_options().mem_limit; > Filed IMPALA-5652 to deprecate it. I tested and confirmed that if you set - hmm i'm not sure, maybe. What about the case where there is no query mem_limit? I guess you are saying the query buffer_pool_limit still kicks in to prevent the hogging of mem since we use lowest_mem() above? But isn't that also true when query mem_limit is larger than process mem_limit? Actually, I'm not sure I understand the problem. Is it a new problem or pre-existing with buffered-block-mgr too? Doesn't the buffer pool version have strictly more limits on the buffer growth (both per query reservation limit along with the process wide buffer pool limit). http://gerrit.cloudera.org:8080/#/c/5801/29/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: PS29, Line 64: starts doesn't it start in Closed? PS29, Line 119: , "and sets 'data_' with pointer to the page's buffer" and then delete the second sentence since part of that is redundant and slightly confusing ("data_ is null" isn't distinct from "not already in memory" but sounds different). PS29, Line 132: / weird line break http://gerrit.cloudera.org:8080/#/c/5801/29/be/src/service/query-options.h File be/src/service/query-options.h: PS29, Line 97: spillable eventually, will these options impact non-"spillable" buffers as well? i.e. nodes that use buffers and not pages? if so, should we remove the word spillable from them? http://gerrit.cloudera.org:8080/#/c/5801/29/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: PS29, Line 120: computeResourceProfile Node Line 122: protected long spillableBufferBytes_ = -1; maybe this should go in ResourceProfile? The reason being that the min reservation is a function of this, so it's pretty tightly coupled, right? what do you think? also do we have coverage where we have a lot of different buffer sizes and also large queries causing mem pressure? basically to test the code that coalesces/breaks buffers in the real environment. Line 631: * data partition. update that comment to explain setting of the buffer size http://gerrit.cloudera.org:8080/#/c/5801/29/fe/src/main/java/org/apache/impala/planner/ResourceProfile.java File fe/src/main/java/org/apache/impala/planner/ResourceProfile.java: PS29, Line 41: [0, Long.MAX_VALUE] are valid values. makes me wonder what valid values for minReservationBytes_ are. Presumably that has to be <= maxReservationBytes_? http://gerrit.cloudera.org:8080/#/c/5801/29/tests/query_test/test_mem_usage_scaling.py File tests/query_test/test_mem_usage_scaling.py: Line 126: 'Q21' : 187, 'Q22' : 125} do you know why some of these increase? is that because of the old small buffers optimization? http://gerrit.cloudera.org:8080/#/c/5801/29/tests/query_test/test_spilling.py File tests/query_test/test_spilling.py: Line 38: # Reduce the IO read size. This reduces the memory required to trigger spilling. i guess now we rely on buffer_pool_limit for that? http://gerrit.cloudera.org:8080/#/c/5801/29/tests/stress/concurrent_select.py File tests/stress/concurrent_select.py: PS29, Line 926: "memory limit exceeded" I guess we have to allow that still until we bring everything else under reservations? -- To view, visit http://gerrit.cloudera.org:8080/5801 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7fc7fe1c04e9dfb1a0c749fb56a5e0f2bf9c6c3e Gerrit-PatchSet: 29 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
