[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10364 ) Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10364 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 Gerrit-Change-Number: 10364 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Wed, 16 May 2018 02:22:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10364 ) Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() .. IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() When a UDF hits a MemLimitExceeded, the query does not immediately abort. Instead, UDFs rely on the caller checking the query_status_ periodically. This means that on some codepaths, UDFs can call SetMemLimitExceeded() many times (e.g. once per row) before the query fragment exits. RuntimeState::SetMemLimitExceeded() currently constructs a MemLimitExceeded Status and dumps it for each call, even if the query has already hit an error. This is expensive and can delay an fragment from exiting when UDFs are repeatedly hitting MemLimitExceeded. This changes SetMemLimitExceeded() to avoid dumping if the query_status_ is already not ok. Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 Reviewed-on: http://gerrit.cloudera.org:8080/10364 Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- M be/src/runtime/runtime-state.cc 1 file changed, 10 insertions(+), 0 deletions(-) Approvals: Tim Armstrong: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/10364 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 Gerrit-Change-Number: 10364 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoram Thanga
[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10364 ) Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() .. Patch Set 2: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2483/ -- To view, visit http://gerrit.cloudera.org:8080/10364 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 Gerrit-Change-Number: 10364 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Tue, 15 May 2018 22:55:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10364 ) Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10364 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 Gerrit-Change-Number: 10364 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Tue, 15 May 2018 21:09:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/10364 ) Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/10364/1/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: http://gerrit.cloudera.org:8080/#/c/10364/1/be/src/runtime/runtime-state.cc@210 PS1, Line 210: // (e.g. once per row) before the fragment aborts. See IMPALA-6997. > I wonder if we should explicitly check that the existing error is TErrorCod We usually record the first error that we hit. If the query status is not ok, then it is aborting. I think it is ok to skip this if we already hit a different error. http://gerrit.cloudera.org:8080/#/c/10364/1/be/src/runtime/runtime-state.cc@211 PS1, Line 211: if (!query_status_.ok()) return; > This probably works in practice but we don't guarantee that Status is threa Changed the code to hold the lock for this check. We can revisit this when we nail down the Status semantics. -- To view, visit http://gerrit.cloudera.org:8080/10364 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 Gerrit-Change-Number: 10364 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Tue, 15 May 2018 19:51:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
Hello Zoram Thanga, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10364 to look at the new patch set (#2). Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() .. IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() When a UDF hits a MemLimitExceeded, the query does not immediately abort. Instead, UDFs rely on the caller checking the query_status_ periodically. This means that on some codepaths, UDFs can call SetMemLimitExceeded() many times (e.g. once per row) before the query fragment exits. RuntimeState::SetMemLimitExceeded() currently constructs a MemLimitExceeded Status and dumps it for each call, even if the query has already hit an error. This is expensive and can delay an fragment from exiting when UDFs are repeatedly hitting MemLimitExceeded. This changes SetMemLimitExceeded() to avoid dumping if the query_status_ is already not ok. Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 --- M be/src/runtime/runtime-state.cc 1 file changed, 10 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/10364/2 -- To view, visit http://gerrit.cloudera.org:8080/10364 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 Gerrit-Change-Number: 10364 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoram Thanga
[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
Zoram Thanga has posted comments on this change. ( http://gerrit.cloudera.org:8080/10364 ) Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() .. Patch Set 1: Ping? -- To view, visit http://gerrit.cloudera.org:8080/10364 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 Gerrit-Change-Number: 10364 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Mon, 14 May 2018 23:27:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
Zoram Thanga has posted comments on this change. ( http://gerrit.cloudera.org:8080/10364 ) Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10364/1/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: http://gerrit.cloudera.org:8080/#/c/10364/1/be/src/runtime/runtime-state.cc@210 PS1, Line 210: // (e.g. once per row) before the fragment aborts. See IMPALA-6997. I wonder if we should explicitly check that the existing error is TErrorCode::MEM_LIMIT_EXCEEDED? I mean, could we come here after hitting some other error condition? -- To view, visit http://gerrit.cloudera.org:8080/10364 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 Gerrit-Change-Number: 10364 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Fri, 11 May 2018 16:23:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10364 ) Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10364/1/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: http://gerrit.cloudera.org:8080/#/c/10364/1/be/src/runtime/runtime-state.cc@211 PS1, Line 211: if (!query_status_.ok()) return; This probably works in practice but we don't guarantee that Status is thread-safe. It's probably best to hold query_status_lock_ when checking this (or do something with atomics, but the lock approach seems fine to me). -- To view, visit http://gerrit.cloudera.org:8080/10364 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 Gerrit-Change-Number: 10364 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 10 May 2018 22:09:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10364 Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() .. IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded() When a UDF hits a MemLimitExceeded, the query does not immediately abort. Instead, UDFs rely on the caller checking the query_status_ periodically. This means that on some codepaths, UDFs can call SetMemLimitExceeded() many times (e.g. once per row) before the query fragment exits. RuntimeState::SetMemLimitExceeded() currently constructs a MemLimitExceeded Status and dumps it for each call, even if the query has already hit an error. This is expensive and can delay an fragment from exiting when UDFs are repeatedly hitting MemLimitExceeded. This changes SetMemLimitExceeded() to avoid dumping if the query_status_ is already not ok. Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 --- M be/src/runtime/runtime-state.cc 1 file changed, 7 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/10364/1 -- To view, visit http://gerrit.cloudera.org:8080/10364 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701 Gerrit-Change-Number: 10364 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell
