[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 23:49:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. IMPALA-7829: Mark a fragment instance as done only after Close() is called As shown in IMPALA-7828. there is some non-determinism on whether the errors detected in FragmentInstanceState::Close() will show up in the final profile sent to the coordinator. The reason is that the current code marks a fragment instance as "done" after ExecInternal() completes but before Close() is called. There is a window between when the final status report is sent and when Close() finishes. This change fixes the problem by not sending the final report until Close() is called. This has no implication on the first row available time for normal queries. It may slightly lengthen the first row available time for DML queries. Testing done: Updated udf-no-expr-rewrite.test to exercise this test Perf run on an 8 node clusters didn't show any regression: TPCH-300 ++---+-++++ | Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | ++---+-++++ | TPCH(_300) | parquet / none / none | 23.94 | -2.05% | 12.55 | -2.62% | ++---+-++++ Small concurrency +-+---+-++++ | Workload| File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +-+---+-++++ | TPCDS-UNMODIFIED(_1000) | parquet / none / none | 6.89| -0.66% | 6.62 | +0.41% | +-+---+-++++ Medium concurrency +-+---+-++++ | Workload| File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +-+---+-++++ | TPCDS-UNMODIFIED(_1000) | parquet / none / none | 55.57 | -1.04% | 55.27 | -0.98% | +-+---+-++++ Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Reviewed-on: http://gerrit.cloudera.org:8080/11939 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test 3 files changed, 12 insertions(+), 19 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3480/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:47:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:47:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 2: Code-Review+2 Carry +2 -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:45:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11939/1/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/11939/1/be/src/runtime/fragment-instance-state.cc@100 PS1, Line 100: ReleaseThreadToken(); > Consider moving to Close()? It's acquired in Prepare() so that feels symmet Done -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 19:44:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Hello Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11939 to look at the new patch set (#2). Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. IMPALA-7829: Mark a fragment instance as done only after Close() is called As shown in IMPALA-7828. there is some non-determinism on whether the errors detected in FragmentInstanceState::Close() will show up in the final profile sent to the coordinator. The reason is that the current code marks a fragment instance as "done" after ExecInternal() completes but before Close() is called. There is a window between when the final status report is sent and when Close() finishes. This change fixes the problem by not sending the final report until Close() is called. This has no implication on the first row available time for normal queries. It may slightly lengthen the first row available time for DML queries. Testing done: Updated udf-no-expr-rewrite.test to exercise this test Perf run on an 8 node clusters didn't show any regression: TPCH-300 ++---+-++++ | Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | ++---+-++++ | TPCH(_300) | parquet / none / none | 23.94 | -2.05% | 12.55 | -2.62% | ++---+-++++ Small concurrency +-+---+-++++ | Workload| File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +-+---+-++++ | TPCDS-UNMODIFIED(_1000) | parquet / none / none | 6.89| -0.66% | 6.62 | +0.41% | +-+---+-++++ Medium concurrency +-+---+-++++ | Workload| File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +-+---+-++++ | TPCDS-UNMODIFIED(_1000) | parquet / none / none | 55.57 | -1.04% | 55.27 | -0.98% | +-+---+-++++ Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 --- M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test 3 files changed, 12 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/11939/2 -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11939/1/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/11939/1/be/src/runtime/fragment-instance-state.cc@100 PS1, Line 100: ReleaseThreadToken(); Consider moving to Close()? It's acquired in Prepare() so that feels symmetrical to me. -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Nov 2018 01:40:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11939 ) Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1381/ : 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/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 16 Nov 2018 03:56:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7829: Mark a fragment instance as done only after Close() is called
Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11939 Change subject: IMPALA-7829: Mark a fragment instance as done only after Close() is called .. IMPALA-7829: Mark a fragment instance as done only after Close() is called As shown in IMPALA-7828. there is some non-determinism on whether the errors detected in FragmentInstanceState::Close() will show up in the final profile sent to the coordinator. The reason is that the current code marks a fragment instance as "done" after ExecInternal() completes but before Close() is called. There is a window between when the final status report is sent and when Close() finishes. This change fixes the problem by not sending the final report until Close() is called. This has no implication on the first row available time for normal queries. It may slightly lengthen the first row available time for DML queries. Testing done: Updated udf-no-expr-rewrite.test to exercise this test Perf run on an 8 node clusters didn't show any regression: TPCH-300 ++---+-++++ | Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | ++---+-++++ | TPCH(_300) | parquet / none / none | 23.94 | -2.05% | 12.55 | -2.62% | ++---+-++++ Small concurrency +-+---+-++++ | Workload| File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +-+---+-++++ | TPCDS-UNMODIFIED(_1000) | parquet / none / none | 6.89| -0.66% | 6.62 | +0.41% | +-+---+-++++ Medium concurrency +-+---+-++++ | Workload| File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +-+---+-++++ | TPCDS-UNMODIFIED(_1000) | parquet / none / none | 55.57 | -1.04% | 55.27 | -0.98% | +-+---+-++++ Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 --- M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test 3 files changed, 11 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/11939/1 -- To view, visit http://gerrit.cloudera.org:8080/11939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I61618854ae3f4e7ef20028dcb0ff5cbcfa8adb01 Gerrit-Change-Number: 11939 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho
