You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jo...@apache.org on 2018/11/21 00:52:49 UTC
[1/2] impala git commit: IMPALA-7829: Mark a fragment instance as
done only after Close() is called
Repository: impala
Updated Branches:
refs/heads/master a78a8d62a -> 5e2dcd25d
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 <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/9ef9daca
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/9ef9daca
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/9ef9daca
Branch: refs/heads/master
Commit: 9ef9dacaf764aac869c8072eee823d1129657cf4
Parents: a78a8d6
Author: Michael Ho <kw...@cloudera.com>
Authored: Thu Nov 8 18:23:43 2018 -0800
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Tue Nov 20 23:49:11 2018 +0000
----------------------------------------------------------------------
be/src/runtime/fragment-instance-state.cc | 20 ++++++++++----------
be/src/runtime/fragment-instance-state.h | 4 ----
.../queries/QueryTest/udf-no-expr-rewrite.test | 7 ++-----
3 files changed, 12 insertions(+), 19 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/9ef9daca/be/src/runtime/fragment-instance-state.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/fragment-instance-state.cc b/be/src/runtime/fragment-instance-state.cc
index b9f9a90..89d388f 100644
--- a/be/src/runtime/fragment-instance-state.cc
+++ b/be/src/runtime/fragment-instance-state.cc
@@ -94,6 +94,10 @@ Status FragmentInstanceState::Exec() {
}
done:
+ // Don't transition to completion until Close() is called as some new errors may be
+ // logged in RuntimeState:error_log_.
+ Close();
+
// Must update the fragment instance state first before updating the 'Query State'.
// Otherwise, there is a race when reading the 'done' flag with GetStatusReport().
// This may lead to the "final" profile being sent with the 'done' flag as false.
@@ -113,9 +117,6 @@ done:
// Tell the managing 'QueryState' that we're done with executing.
query_state_->DoneExecuting();
}
- // call this before Close() to make sure the thread token got released
- Finalize(status);
- Close();
return status;
}
@@ -339,6 +340,12 @@ Status FragmentInstanceState::ExecInternal() {
void FragmentInstanceState::Close() {
DCHECK(runtime_state_ != nullptr);
+ // If we haven't already released this thread token in Prepare(), release
+ // it before calling Close().
+ if (fragment_ctx_.fragment.output_sink.type != TDataSinkType::PLAN_ROOT_SINK) {
+ ReleaseThreadToken();
+ }
+
// guard against partially-finished Prepare()
if (sink_ != nullptr) sink_->Close(runtime_state_);
@@ -440,13 +447,6 @@ void FragmentInstanceState::UpdateState(const StateEvent event)
if (next_state != current_state) current_state_.Store(next_state);
}
-void FragmentInstanceState::Finalize(const Status& status) {
- if (fragment_ctx_.fragment.output_sink.type != TDataSinkType::PLAN_ROOT_SINK) {
- // if we haven't already release this thread token in Prepare(), release it now
- ReleaseThreadToken();
- }
-}
-
void FragmentInstanceState::ReleaseThreadToken() {
DCHECK(runtime_state_ != nullptr);
DCHECK(runtime_state_->resource_pool() != nullptr);
http://git-wip-us.apache.org/repos/asf/impala/blob/9ef9daca/be/src/runtime/fragment-instance-state.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/fragment-instance-state.h b/be/src/runtime/fragment-instance-state.h
index 7636055..c7b8d79 100644
--- a/be/src/runtime/fragment-instance-state.h
+++ b/be/src/runtime/fragment-instance-state.h
@@ -268,10 +268,6 @@ class FragmentInstanceState {
/// concurrently.
void UpdateState(const StateEvent event);
- /// Called when execution is complete to finalize counters. Must be called only once.
- /// Can handle partially-finished Prepare().
- void Finalize(const Status& status);
-
/// Releases the thread token for this fragment executor. Can handle
/// partially-finished Prepare().
void ReleaseThreadToken();
http://git-wip-us.apache.org/repos/asf/impala/blob/9ef9daca/testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test b/testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test
index 364882e..d14723d 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/udf-no-expr-rewrite.test
@@ -22,9 +22,6 @@ select mem_test_leaks(100);
bigint
---- RESULTS
100
-#---- ERRORS
-#TODO: Fix the non-determinisim below. IMPALA-7829.
-#A fragment instance's last status report may be sent before calling Close() which is
-#where the memory leak is detected. So, we don't always get the following error message.
-#UDF WARNING: Memory leaked via FunctionContext::Allocate(), 100 bytes leaked via FunctionContext::TrackAllocation()
+---- ERRORS
+UDF WARNING: Memory leaked via FunctionContext::Allocate(), 100 bytes leaked via FunctionContext::TrackAllocation()
====
[2/2] impala git commit: IMPALA-7873: Fix flakiness in
TestExchangeMemUsage
Posted by jo...@apache.org.
IMPALA-7873: Fix flakiness in TestExchangeMemUsage
IMPALA-7367 reduced the memory requirement of the query tested in
test_exchange_mem_usage_scaling. This change reduces the mem limit
to ensure that the query runs out of memory as expected.
Testing:
Ran the test 100 times in a loop without any failures.
Change-Id: Ib2f063fb88ebf0c7f994b55ecfc860d81726fdd8
Reviewed-on: http://gerrit.cloudera.org:8080/11965
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/5e2dcd25
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/5e2dcd25
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/5e2dcd25
Branch: refs/heads/master
Commit: 5e2dcd25d855711508088fd2f9f6e2bf7523fdfc
Parents: 9ef9dac
Author: poojanilangekar <po...@cloudera.com>
Authored: Tue Nov 20 11:38:49 2018 -0800
Committer: Joe McDonnell <jo...@cloudera.com>
Committed: Wed Nov 21 00:08:25 2018 +0000
----------------------------------------------------------------------
.../functional-query/queries/QueryTest/exchange-mem-scaling.test | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/5e2dcd25/testdata/workloads/functional-query/queries/QueryTest/exchange-mem-scaling.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/exchange-mem-scaling.test b/testdata/workloads/functional-query/queries/QueryTest/exchange-mem-scaling.test
index 84ffe34..4affa6e 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/exchange-mem-scaling.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/exchange-mem-scaling.test
@@ -4,7 +4,7 @@
# it hits the memory limit in the exchange node when allocating receiver-side
# buffers. It's also possible but less likely that this will hit a memory limit
# in the scan nodes.
-set mem_limit=200m;
+set mem_limit=170m;
set num_scanner_threads=1;
select *
from tpch_parquet.lineitem l1