[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 7: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 02 Aug 2018 01:46:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2899/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 01 Aug 2018 22:30:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 01 Aug 2018 21:05:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/135/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 01 Aug 2018 19:19:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 5: (4 comments) Sorry for the delay, lost track of this one http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/bufferpool/buffer-pool.h File be/src/runtime/bufferpool/buffer-pool.h: http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/bufferpool/buffer-pool.h@178 PS5, Line 178: check_soft_mem_limit > nit: mem_limit_mode Done http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/bufferpool/reservation-tracker.cc File be/src/runtime/bufferpool/reservation-tracker.cc: http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/bufferpool/reservation-tracker.cc@84 PS5, Line 84: MemLimit::HARD > I'm not too familiar with the ReservationTracker stuff, so maybe this is wr I think either way is equivalent, but mem_limit_mode_ probably makes more intuitive sense. http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/mem-tracker-types.h File be/src/runtime/mem-tracker-types.h: http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/mem-tracker-types.h@18 PS5, Line 18: : #ifndef IMPALA_RUNTIME_MEM_TRACKER_TYPES_H : #define IMPALA_RUNTIME_MEM_TRACKER_TYPES_H : > we've moved to '#pragma once' in Kudu for this, no such preference in impal I don't think it's been on our radar. Technically the Google C++ style guide seems to not allow it, but probably it makes sense - I think all our compilers support it. I'll start a discussion on the mailing list and update our style guide on the wiki. https://google.github.io/styleguide/cppguide.html#Windows_Code http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/mem-tracker.cc@41 PS5, Line 41: DEFINE_double > maybe make this hidden if we dont expect users to configure it initially? Done -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 01 Aug 2018 18:46:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Hello Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10988 to look at the new patch set (#6). Change subject: IMPALA-7297: soft memory limit for reservation increases .. IMPALA-7297: soft memory limit for reservation increases https://goo.gl/N9LgQt summarises the memory problems I'm trying to solve here. Reject reservation increases that would leave < 10% of the memory available under a query or process mem_limit, which put the query or impalad into a state with a heightened chance of OOM. This avoids OOM in some scenarios where reserved memory is increased too aggressively and starves non-reserved memory. One way of thinking about this that it's extending the 80% limit heuristic for reserved memory so that there's a soft limit of min(80%, 90% - non-reserved memory consumption) A follow-on will use this for scanner threads to prevent starting scanner threads that may exceed the soft limit. Testing: Added unit tests for MemTracker and for ReservationTracker. Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac --- M be/src/exec/exchange-node.cc M be/src/rpc/impala-service-pool.cc M be/src/runtime/bufferpool/buffer-pool-internal.h M be/src/runtime/bufferpool/buffer-pool.cc M be/src/runtime/bufferpool/buffer-pool.h M be/src/runtime/bufferpool/reservation-tracker-test.cc M be/src/runtime/bufferpool/reservation-tracker.cc M be/src/runtime/bufferpool/reservation-tracker.h M be/src/runtime/initial-reservations.cc M be/src/runtime/mem-pool-test.cc M be/src/runtime/mem-tracker-test.cc A be/src/runtime/mem-tracker-types.h M be/src/runtime/mem-tracker.cc M be/src/runtime/mem-tracker.h M be/src/runtime/query-state.cc M be/src/runtime/runtime-state.cc M be/src/udf/udf.cc 17 files changed, 313 insertions(+), 151 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/10988/6 -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/bufferpool/buffer-pool.h File be/src/runtime/bufferpool/buffer-pool.h: http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/bufferpool/buffer-pool.h@178 PS5, Line 178: check_soft_mem_limit nit: mem_limit_mode http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/bufferpool/reservation-tracker.cc File be/src/runtime/bufferpool/reservation-tracker.cc: http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/bufferpool/reservation-tracker.cc@84 PS5, Line 84: MemLimit::HARD I'm not too familiar with the ReservationTracker stuff, so maybe this is wrong, but should this be using mem_limit_mode instead of HARD? http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/mem-tracker-types.h File be/src/runtime/mem-tracker-types.h: http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/mem-tracker-types.h@18 PS5, Line 18: : #ifndef IMPALA_RUNTIME_MEM_TRACKER_TYPES_H : #define IMPALA_RUNTIME_MEM_TRACKER_TYPES_H : we've moved to '#pragma once' in Kudu for this, no such preference in impala? http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/10988/5/be/src/runtime/mem-tracker.cc@41 PS5, Line 41: DEFINE_double maybe make this hidden if we dont expect users to configure it initially? -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Jul 2018 23:04:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 5: > Also, doesn't this somewhat rely on the scanner threads being quicker to > claim the memory than the other threads? If the other threads already > expanded to their full 80%, this won't help, right? Or is the hope that those > other threads are cycling their memory often enough that, as scanner threads > encroach on the boundary, they'll have a chance to free some memory and not > re-allocate? There is a follow-on patch I'm working on to make the scanner threads a bit smarter about whether they start based on available memory and reduce the raciness. It still will be somewhat racy. It is typical though that the scans grab memory first - e.g. the scans all start immediately and the joins, aggregations and sorts gradually balloon as they accumulate memory from the scans. I think one fundamental design decision we made that impacts this is that there's no kind of pre-emption - one operator can't force another operator to scale down memory consumption if it consumed too much. -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 26 Jul 2018 23:04:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 5: I think you have it right. The 80/20 heuristic has been around since Impala 2.0. I don't think there's anything special about the numbers but it's proven difficult to change without negatively impacting some queries (either worse performance or higher memory requirements) One additional bit of context is that long-term we want to move most or all execution memory under reservations, so this is a bit of a stopgap. I also have a follow-on patch that makes HDFS + Kudu scans check the soft limit to decide whether to start or stop scanner threads. HDFS scans use a mix of reserved and non-reserved memory. Kudu uses all non-reserved memory. So that's a case where the alternative of reducing the reserved memory doesn't help us. -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 26 Jul 2018 22:59:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 5: Haven't looked at the code yet, just trying to understand the overall design. Mind if I repeat it back to you and you can tell me if I got it right? Current state of the world: - - Each query has a user-specified mem limit - We calculate a "reservation limit" as 80% of that mem limit (except for in some corner cases for tiny mem limits where there is a 100MB threshold) - Operators that support spilling all compete to try to snag chunks from the reserved pool. If such an allocation fails, those operators are expected to spill, etc. - Other operators like scans aren't reservation-aware, so they just allocate when they feel like it. -- This normally comes out of the 20% pool made up of (mem_limit - reservation_limit) -- If these operators fail to acquire their memory, they'll just fail the query. - If the reserved operators collectively haven't used up their allocated 80%, the other operators are allowed to slop into the reserved pool so long as mem_limit isn't exceeded. - If that "slopping" has happened, the reserved operators will still keep allocating chunks and likely put the query exactly at the mem limit, at which point any further allocation from the non-reserved operators will fail the query. This patch: --- Same as above, except: - if the reserved operators try to allocate but the overall mem_limit is close to being exceeded (because naughty operators slopped over into their allocation or close to doing so) they'll just pretend that the reservation amount was smaller than it actually is. Put another way, is this patch basically saying "even though we reserved 80%, we'll actually dynamically reduce that 80% allocation if it looks like our other operators use more than we had hoped"? I guess the piece I'm not 100% understanding is why this is a separate "soft" limit vs just some kind of dynamic adjustment of the reservation pool size based on observed "slop" size? Also, doesn't this somewhat rely on the scanner threads being quicker to claim the memory than the other threads? If the other threads already expanded to their full 80%, this won't help, right? Or is the hope that those other threads are cycling their memory often enough that, as scanner threads encroach on the boundary, they'll have a chance to free some memory and not re-allocate? -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 26 Jul 2018 01:34:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/67/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 26 Jul 2018 00:03:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 5: Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/67/ Running initial code review checks. This is experimental - please report any issues to tarmstr...@cloudera.com or on this JIRA: IMPALA-7317 -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 25 Jul 2018 23:29:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. Patch Set 5: Todd, I'd be interested if you could take a look. I know Kudu has a soft memory limit concept so maybe you have some thoughts on this. -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Jul 2018 23:30:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7297: soft memory limit for reservation increases
Tim Armstrong has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10988 ) Change subject: IMPALA-7297: soft memory limit for reservation increases .. IMPALA-7297: soft memory limit for reservation increases https://goo.gl/N9LgQt summarises the memory problems I'm trying to solve here. Reject reservation increases that would leave < 10% of the memory available under a query or process mem_limit, which put the query or impalad into a state with a heightened chance of OOM. This avoids OOM in some scenarios where reserved memory is increased too aggressively and starves non-reserved memory. One way of thinking about this that it's extending the 80% limit heuristic for reserved memory so that there's a soft limit of min(80%, 90% - non-reserved memory consumption) A follow-on will use this for scanner threads to prevent starting scanner threads that may exceed the soft limit. Testing: Added unit tests for MemTracker and for ReservationTracker. Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac --- M be/src/exec/exchange-node.cc M be/src/rpc/impala-service-pool.cc M be/src/runtime/bufferpool/buffer-pool-internal.h M be/src/runtime/bufferpool/buffer-pool.cc M be/src/runtime/bufferpool/buffer-pool.h M be/src/runtime/bufferpool/reservation-tracker-test.cc M be/src/runtime/bufferpool/reservation-tracker.cc M be/src/runtime/bufferpool/reservation-tracker.h M be/src/runtime/initial-reservations.cc M be/src/runtime/mem-pool-test.cc M be/src/runtime/mem-tracker-test.cc A be/src/runtime/mem-tracker-types.h M be/src/runtime/mem-tracker.cc M be/src/runtime/mem-tracker.h M be/src/runtime/query-state.cc M be/src/runtime/runtime-state.cc M be/src/udf/udf.cc 17 files changed, 313 insertions(+), 151 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/10988/5 -- To view, visit http://gerrit.cloudera.org:8080/10988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I39dcf2dd870a59c8b8cda4488fe41ce936d715ac Gerrit-Change-Number: 10988 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong
