You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2017/08/30 04:08:49 UTC
[1/4] incubator-impala git commit: IMPALA-5830:
SET_DENY_RESERVATION_PROBABILITY test
Repository: incubator-impala
Updated Branches:
refs/heads/master 6a9ca345b -> cc4816b3d
IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test
Add a targeted test that confirms that setting the query option will
force spilling.
Testing:
Ran test_spilling locally.
Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce
Reviewed-on: http://gerrit.cloudera.org:8080/7809
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/caefd861
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/caefd861
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/caefd861
Branch: refs/heads/master
Commit: caefd86136e7d8d5d3cd390402abf3370dc6cc6f
Parents: 6a9ca34
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Thu Aug 24 13:26:46 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Tue Aug 29 23:01:10 2017 +0000
----------------------------------------------------------------------
.../QueryTest/disable-unsafe-spills.test | 11 ----
.../QueryTest/spilling-query-options.test | 54 ++++++++++++++++++++
tests/common/impala_test_suite.py | 2 +-
tests/query_test/test_spilling.py | 41 +++++++++++----
4 files changed, 85 insertions(+), 23 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/caefd861/testdata/workloads/functional-query/queries/QueryTest/disable-unsafe-spills.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/disable-unsafe-spills.test b/testdata/workloads/functional-query/queries/QueryTest/disable-unsafe-spills.test
deleted file mode 100644
index a9b8f35..0000000
--- a/testdata/workloads/functional-query/queries/QueryTest/disable-unsafe-spills.test
+++ /dev/null
@@ -1,11 +0,0 @@
-====
----- QUERY
-# tpch_avro does not have stats computed, so if we set disable_unsafe_spills we should
-# not spill to disk.
-set disable_unsafe_spills=true;
-set buffer_pool_limit=40m;
-select distinct *
-from tpch_avro.orders
----- CATCH
-Could not free memory by spilling to disk: spilling was disabled by planner. Re-enable spilling by setting the query option DISABLE_UNSAFE_SPILLS=false
-====
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/caefd861/testdata/workloads/functional-query/queries/QueryTest/spilling-query-options.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/spilling-query-options.test b/testdata/workloads/functional-query/queries/QueryTest/spilling-query-options.test
new file mode 100644
index 0000000..5b9a641
--- /dev/null
+++ b/testdata/workloads/functional-query/queries/QueryTest/spilling-query-options.test
@@ -0,0 +1,54 @@
+====
+---- QUERY
+# tpch_avro does not have stats computed, so if we set disable_unsafe_spills we should
+# not spill to disk.
+set disable_unsafe_spills=true;
+set buffer_pool_limit=40m;
+select distinct *
+from tpch_avro.orders
+---- CATCH
+Could not free memory by spilling to disk: spilling was disabled by planner. Re-enable spilling by setting the query option DISABLE_UNSAFE_SPILLS=false
+====
+---- QUERY
+# IMPALA-5823: make sure that SET_DENY_RESERVATION_PROBABILITY takes effect on PREPARE.
+set debug_action="-1:PREPARE:SET_DENY_RESERVATION_PROBABILITY@1.0";
+select count(*) from (
+ select distinct o_orderdate, o_custkey, o_comment
+ from tpch_parquet.orders where o_orderkey < 500000) v;
+---- TYPES
+BIGINT
+---- RESULTS
+124999
+---- RUNTIME_PROFILE
+row_regex: .*SpilledPartitions: .* \([1-9][0-9]*\)
+row_regex: .*RowsPassedThrough: .* \([1-9][0-9]*\)
+====
+---- QUERY
+# IMPALA-5823: make sure that SET_DENY_RESERVATION_PROBABILITY takes effect on OPEN.
+set debug_action="-1:OPEN:SET_DENY_RESERVATION_PROBABILITY@1.0";
+select count(*) from (
+ select distinct o_orderdate, o_custkey, o_comment
+ from tpch_parquet.orders where o_orderkey < 500000) v;
+---- TYPES
+BIGINT
+---- RESULTS
+124999
+---- RUNTIME_PROFILE
+row_regex: .*SpilledPartitions: .* \([1-9][0-9]*\)
+row_regex: .*RowsPassedThrough: .* \([1-9][0-9]*\)
+====
+---- QUERY
+# IMPALA-5823: make sure that SET_DENY_RESERVATION_PROBABILITY takes effect on GETNEXT.
+# This won't affect the merge aggregation, which accumulates its memory in Open(), but
+# will affect the streaming aggregation.
+set debug_action="-1:GETNEXT:SET_DENY_RESERVATION_PROBABILITY@1.0";
+select count(*) from (
+ select distinct o_orderdate, o_custkey, o_comment
+ from tpch_parquet.orders where o_orderkey < 500000) v;
+---- TYPES
+BIGINT
+---- RESULTS
+124999
+---- RUNTIME_PROFILE
+row_regex: .*RowsPassedThrough: .* \([1-9][0-9]*\)
+====
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/caefd861/tests/common/impala_test_suite.py
----------------------------------------------------------------------
diff --git a/tests/common/impala_test_suite.py b/tests/common/impala_test_suite.py
index d5841b2..1b7d043 100644
--- a/tests/common/impala_test_suite.py
+++ b/tests/common/impala_test_suite.py
@@ -210,7 +210,7 @@ class ImpalaTestSuite(BaseTestSuite):
if not query_option in self.default_query_options:
continue
default_val = self.default_query_options[query_option]
- query_str = 'SET '+ query_option + '=' + default_val + ';'
+ query_str = 'SET '+ query_option + '="' + default_val + '"'
try:
impalad_client.execute(query_str)
except Exception as e:
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/caefd861/tests/query_test/test_spilling.py
----------------------------------------------------------------------
diff --git a/tests/query_test/test_spilling.py b/tests/query_test/test_spilling.py
index c818b65..0b36429 100644
--- a/tests/query_test/test_spilling.py
+++ b/tests/query_test/test_spilling.py
@@ -30,14 +30,14 @@ DEBUG_ACTION_DIMS = [None,
@pytest.mark.xfail(pytest.config.option.testing_remote_cluster,
reason='Queries may not spill on larger clusters')
-class TestSpilling(ImpalaTestSuite):
+class TestSpillingDebugActionDimensions(ImpalaTestSuite):
@classmethod
def get_workload(self):
return 'functional-query'
@classmethod
def add_test_dimensions(cls):
- super(TestSpilling, cls).add_test_dimensions()
+ super(TestSpillingDebugActionDimensions, cls).add_test_dimensions()
cls.ImpalaTestMatrix.clear_constraints()
cls.ImpalaTestMatrix.add_dimension(create_parquet_dimension('tpch'))
# Tests are calibrated so that they can execute and spill with this page size.
@@ -60,19 +60,38 @@ class TestSpilling(ImpalaTestSuite):
"""Test spilling null-aware anti-joins"""
self.run_test_case('QueryTest/spilling-naaj', vector)
+ def test_spilling_sorts_exhaustive(self, vector):
+ if self.exploration_strategy() != 'exhaustive':
+ pytest.skip("only run large sorts on exhaustive")
+ self.run_test_case('QueryTest/spilling-sorts-exhaustive', vector)
+
+
+@pytest.mark.xfail(pytest.config.option.testing_remote_cluster,
+ reason='Queries may not spill on larger clusters')
+class TestSpillingNoDebugActionDimensions(ImpalaTestSuite):
+ """Spilling tests to which we don't want to apply the debug_action dimension."""
+ @classmethod
+ def get_workload(self):
+ return 'functional-query'
+
+ @classmethod
+ def add_test_dimensions(cls):
+ super(TestSpillingNoDebugActionDimensions, cls).add_test_dimensions()
+ cls.ImpalaTestMatrix.clear_constraints()
+ cls.ImpalaTestMatrix.add_dimension(create_parquet_dimension('tpch'))
+ # Tests are calibrated so that they can execute and spill with this page size.
+ cls.ImpalaTestMatrix.add_dimension(
+ create_exec_option_dimension_from_dict({'default_spillable_buffer_size' : ['256k']}))
+
def test_spilling_naaj_no_deny_reservation(self, vector):
"""
Null-aware anti-join tests that depend on getting more than the minimum reservation
and therefore will not reliably pass with the deny reservation debug action enabled.
"""
- if vector.get_value('exec_option')['debug_action'] is None:
- self.run_test_case('QueryTest/spilling-naaj-no-deny-reservation', vector)
+ self.run_test_case('QueryTest/spilling-naaj-no-deny-reservation', vector)
- def test_spilling_sorts_exhaustive(self, vector):
- if self.exploration_strategy() != 'exhaustive':
- pytest.skip("only run large sorts on exhaustive")
- self.run_test_case('QueryTest/spilling-sorts-exhaustive', vector)
+ def test_spilling_query_options(self, vector):
+ """Test that spilling-related query options work end-to-end. These tests rely on
+ setting debug_action to alternative values via query options."""
+ self.run_test_case('QueryTest/spilling-query-options', vector)
- def test_disable_unsafe_spills(self, vector):
- """Test that the disable_unsafe_spills query options works end-to-end."""
- self.run_test_case('QueryTest/disable-unsafe-spills', vector)
[3/4] incubator-impala git commit: IMPALA-5857: avoid invalid free of
hedged read metrics
Posted by ta...@apache.org.
IMPALA-5857: avoid invalid free of hedged read metrics
The libHdfs API documents that the output parameter is unchanged on
error, therefore we do not need to attempt to free it on error.
Testing:
The bug only reproduced under stress. I don't know how to trigger this
error path yet.
Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
Reviewed-on: http://gerrit.cloudera.org:8080/7885
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/d5670d6b
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/d5670d6b
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/d5670d6b
Branch: refs/heads/master
Commit: d5670d6b912c65c9156d165603d65336d96a30a1
Parents: 35f5c7b
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Tue Aug 29 15:29:50 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Wed Aug 30 03:36:41 2017 +0000
----------------------------------------------------------------------
be/src/runtime/disk-io-mgr-scan-range.cc | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d5670d6b/be/src/runtime/disk-io-mgr-scan-range.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/disk-io-mgr-scan-range.cc b/be/src/runtime/disk-io-mgr-scan-range.cc
index f9f96d0..df74fcc 100644
--- a/be/src/runtime/disk-io-mgr-scan-range.cc
+++ b/be/src/runtime/disk-io-mgr-scan-range.cc
@@ -350,14 +350,14 @@ void DiskIoMgr::ScanRange::Close() {
// Update Hedged Read Metrics.
// We call it only if the --use_hdfs_pread flag is set, to avoid having the
// libhdfs client malloc and free a hdfsHedgedReadMetrics object unnecessarily
- // otherwise.
+ // otherwise. 'hedged_metrics' is only set upon success.
struct hdfsHedgedReadMetrics* hedged_metrics;
int success = hdfsGetHedgedReadMetrics(fs_, &hedged_metrics);
if (success == 0) {
ImpaladMetrics::HEDGED_READ_OPS->set_value(hedged_metrics->hedgedReadOps);
ImpaladMetrics::HEDGED_READ_OPS_WIN->set_value(hedged_metrics->hedgedReadOpsWin);
+ hdfsFreeHedgedReadMetrics(hedged_metrics);
}
- hdfsFreeHedgedReadMetrics(hedged_metrics);
}
if (num_remote_bytes_ > 0) {
[2/4] incubator-impala git commit: IMPALA-4856: Rename thrift-deps to
gen-deps
Posted by ta...@apache.org.
IMPALA-4856: Rename thrift-deps to gen-deps
As a preparation to start generating Protobuf files
for IMPALA-4856, this change introduces a new build
target "gen-deps" which serves as an umbrella for all
build targets of generated code. For now, it only
includes thrift-deps and protobuf targets will be added
in the future.
Change-Id: I360c63773efdeab4c26ca96b915e0c8d0ce2b9c9
Reviewed-on: http://gerrit.cloudera.org:8080/7851
Reviewed-by: Lars Volker <lv...@cloudera.com>
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/35f5c7bd
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/35f5c7bd
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/35f5c7bd
Branch: refs/heads/master
Commit: 35f5c7bd37e7151f7f71d3136e9a16da6e85d582
Parents: caefd86
Author: Michael Ho <kw...@cloudera.com>
Authored: Fri Aug 25 20:24:36 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Wed Aug 30 00:26:52 2017 +0000
----------------------------------------------------------------------
CMakeLists.txt | 7 +++++--
be/src/catalog/CMakeLists.txt | 2 +-
be/src/codegen/CMakeLists.txt | 2 +-
be/src/common/CMakeLists.txt | 4 ++--
be/src/exec/CMakeLists.txt | 2 +-
be/src/experiments/CMakeLists.txt | 2 +-
be/src/exprs/CMakeLists.txt | 4 ++--
be/src/rpc/CMakeLists.txt | 2 +-
be/src/runtime/CMakeLists.txt | 2 +-
be/src/runtime/bufferpool/CMakeLists.txt | 2 +-
be/src/scheduling/CMakeLists.txt | 2 +-
be/src/service/CMakeLists.txt | 4 ++--
be/src/statestore/CMakeLists.txt | 2 +-
be/src/testutil/CMakeLists.txt | 8 ++++----
be/src/transport/CMakeLists.txt | 2 +-
be/src/udf/CMakeLists.txt | 4 ++--
be/src/udf_samples/CMakeLists.txt | 8 ++++----
be/src/util/CMakeLists.txt | 2 +-
ext-data-source/CMakeLists.txt | 2 +-
19 files changed, 33 insertions(+), 30 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 954193e..e5c2fdb 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -349,9 +349,12 @@ add_subdirectory(be)
add_subdirectory(fe)
add_subdirectory(ext-data-source)
+# Build target for all generated files which most backend code depends on
+add_custom_target(gen-deps ALL DEPENDS thrift-deps)
+
add_custom_target(tarballs ALL DEPENDS shell_tarball)
-add_custom_target(shell_tarball DEPENDS thrift-deps
+add_custom_target(shell_tarball DEPENDS gen-deps
COMMAND "${CMAKE_SOURCE_DIR}/shell/make_shell_tarball.sh"
)
@@ -360,7 +363,7 @@ add_custom_target(cscope ALL
)
if (DEFINED ENV{IMPALA_LZO} AND EXISTS $ENV{IMPALA_LZO})
- add_custom_target(impala-lzo ALL DEPENDS thrift-deps
+ add_custom_target(impala-lzo ALL DEPENDS gen-deps
COMMAND $ENV{IMPALA_LZO}/build.sh ${CMAKE_BUILD_TYPE} ${CMAKE_SOURCE_DIR}
$ENV{IMPALA_TOOLCHAIN}
)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/catalog/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/catalog/CMakeLists.txt b/be/src/catalog/CMakeLists.txt
index ece6c58..7debb91 100644
--- a/be/src/catalog/CMakeLists.txt
+++ b/be/src/catalog/CMakeLists.txt
@@ -24,4 +24,4 @@ add_library(Catalog
catalog-util.cc
catalogd-main.cc
)
-add_dependencies(Catalog thrift-deps)
+add_dependencies(Catalog gen-deps)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/codegen/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/codegen/CMakeLists.txt b/be/src/codegen/CMakeLists.txt
index edd78a0..e640009 100644
--- a/be/src/codegen/CMakeLists.txt
+++ b/be/src/codegen/CMakeLists.txt
@@ -35,7 +35,7 @@ add_library(CodeGen
${IR_SSE_C_FILE}
${IR_NO_SSE_C_FILE}
)
-add_dependencies(CodeGen thrift-deps gen_ir_descriptions)
+add_dependencies(CodeGen gen-deps gen_ir_descriptions)
# output cross compile to ir metadata
set(IR_DESC_GEN_OUTPUT
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/common/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/common/CMakeLists.txt b/be/src/common/CMakeLists.txt
index dd1e441..8ba8ca9 100644
--- a/be/src/common/CMakeLists.txt
+++ b/be/src/common/CMakeLists.txt
@@ -41,12 +41,12 @@ add_custom_command(
COMMENT "Generating the version.cc file"
VERBATIM
)
-add_dependencies(Common thrift-deps)
+add_dependencies(Common gen-deps)
add_library(GlobalFlags
global-flags.cc
)
-add_dependencies(GlobalFlags thrift-deps)
+add_dependencies(GlobalFlags gen-deps)
ADD_BE_TEST(atomic-test)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/exec/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/exec/CMakeLists.txt b/be/src/exec/CMakeLists.txt
index a94a38d..b5f7d3e 100644
--- a/be/src/exec/CMakeLists.txt
+++ b/be/src/exec/CMakeLists.txt
@@ -96,7 +96,7 @@ add_library(Exec
unnest-node.cc
)
-add_dependencies(Exec thrift-deps)
+add_dependencies(Exec gen-deps)
ADD_BE_TEST(zigzag-test)
ADD_BE_TEST(hash-table-test)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/experiments/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/experiments/CMakeLists.txt b/be/src/experiments/CMakeLists.txt
index e4d4895..b4f0c49 100644
--- a/be/src/experiments/CMakeLists.txt
+++ b/be/src/experiments/CMakeLists.txt
@@ -24,7 +24,7 @@ set(EXECUTABLE_OUTPUT_PATH "${BUILD_OUTPUT_ROOT_DIRECTORY}/experiments")
add_library(Experiments
data-provider.cc
)
-add_dependencies(Experiments thrift-deps)
+add_dependencies(Experiments gen-deps)
add_executable(data-provider-test data-provider-test.cc)
add_executable(tuple-splitter-test tuple-splitter-test.cc)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/exprs/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/exprs/CMakeLists.txt b/be/src/exprs/CMakeLists.txt
index 1fe3bcb..cff391c 100644
--- a/be/src/exprs/CMakeLists.txt
+++ b/be/src/exprs/CMakeLists.txt
@@ -64,13 +64,13 @@ add_library(Exprs
utility-functions.cc
utility-functions-ir.cc
)
-add_dependencies(Exprs thrift-deps gen_ir_descriptions)
+add_dependencies(Exprs gen-deps gen_ir_descriptions)
ADD_BE_TEST(expr-test)
ADD_BE_TEST(expr-codegen-test)
# expr-codegen-test includes test IR functions
COMPILE_TO_IR(expr-codegen-test.cc)
-add_dependencies(expr-codegen-test-ir thrift-deps)
+add_dependencies(expr-codegen-test-ir gen-deps)
ADD_UDF_TEST(aggregate-functions-test)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/rpc/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/rpc/CMakeLists.txt b/be/src/rpc/CMakeLists.txt
index 2386a85..d837f6c 100644
--- a/be/src/rpc/CMakeLists.txt
+++ b/be/src/rpc/CMakeLists.txt
@@ -30,7 +30,7 @@ add_library(Rpc
thrift-server.cc
thrift-thread.cc
)
-add_dependencies(Rpc thrift-deps)
+add_dependencies(Rpc gen-deps)
ADD_BE_TEST(thrift-util-test)
ADD_BE_TEST(thrift-server-test)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/runtime/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/runtime/CMakeLists.txt b/be/src/runtime/CMakeLists.txt
index c9aacc9..ad9f074 100644
--- a/be/src/runtime/CMakeLists.txt
+++ b/be/src/runtime/CMakeLists.txt
@@ -72,7 +72,7 @@ add_library(Runtime
tuple-row.cc
tmp-file-mgr.cc
)
-add_dependencies(Runtime thrift-deps)
+add_dependencies(Runtime gen-deps)
# This test runs forever so should not be part of 'make test'
add_executable(disk-io-mgr-stress-test disk-io-mgr-stress-test.cc)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/runtime/bufferpool/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/runtime/bufferpool/CMakeLists.txt b/be/src/runtime/bufferpool/CMakeLists.txt
index 4b1778b..ce68b07 100644
--- a/be/src/runtime/bufferpool/CMakeLists.txt
+++ b/be/src/runtime/bufferpool/CMakeLists.txt
@@ -29,7 +29,7 @@ add_library(BufferPool
suballocator.cc
system-allocator.cc
)
-add_dependencies(BufferPool thrift-deps)
+add_dependencies(BufferPool gen-deps)
ADD_BE_TEST(buffer-allocator-test)
ADD_BE_TEST(buffer-pool-test)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/scheduling/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/scheduling/CMakeLists.txt b/be/src/scheduling/CMakeLists.txt
index e80ea2b..024e6e1 100644
--- a/be/src/scheduling/CMakeLists.txt
+++ b/be/src/scheduling/CMakeLists.txt
@@ -31,7 +31,7 @@ add_library(Scheduling STATIC
scheduler-test-util.cc
scheduler.cc
)
-add_dependencies(Scheduling thrift-deps)
+add_dependencies(Scheduling gen-deps)
ADD_BE_TEST(scheduler-test)
ADD_BE_TEST(backend-config-test)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/service/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/service/CMakeLists.txt b/be/src/service/CMakeLists.txt
index cfb0bf3..ab51740 100644
--- a/be/src/service/CMakeLists.txt
+++ b/be/src/service/CMakeLists.txt
@@ -36,13 +36,13 @@ add_library(Service
child-query.cc
impalad-main.cc
)
-add_dependencies(Service thrift-deps)
+add_dependencies(Service gen-deps)
# this shared library provides Impala executor functionality to FE test.
add_library(fesupport SHARED
fe-support.cc
)
-add_dependencies(fesupport thrift-deps)
+add_dependencies(fesupport gen-deps)
target_link_libraries(fesupport ${IMPALA_LINK_LIBS_DYNAMIC_TARGETS})
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/statestore/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/statestore/CMakeLists.txt b/be/src/statestore/CMakeLists.txt
index 50e3703..313b2ec 100644
--- a/be/src/statestore/CMakeLists.txt
+++ b/be/src/statestore/CMakeLists.txt
@@ -28,6 +28,6 @@ add_library(Statestore
statestore-subscriber.cc
statestored-main.cc
)
-add_dependencies(Statestore thrift-deps)
+add_dependencies(Statestore gen-deps)
ADD_BE_TEST(statestore-test)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/testutil/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/testutil/CMakeLists.txt b/be/src/testutil/CMakeLists.txt
index f6d2bcd..310ab37 100644
--- a/be/src/testutil/CMakeLists.txt
+++ b/be/src/testutil/CMakeLists.txt
@@ -32,13 +32,13 @@ add_library(TestUtil
test-udas.cc
test-udfs.cc
)
-add_dependencies(TestUtil thrift-deps)
+add_dependencies(TestUtil gen-deps)
add_library(TestUdfs SHARED test-udfs.cc)
-add_dependencies(TestUdfs thrift-deps)
+add_dependencies(TestUdfs gen-deps)
COMPILE_TO_IR(test-udfs.cc)
-add_dependencies(test-udfs-ir thrift-deps)
+add_dependencies(test-udfs-ir gen-deps)
add_library(TestUdas SHARED test-udas.cc)
-add_dependencies(TestUdas thrift-deps)
+add_dependencies(TestUdas gen-deps)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/transport/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/transport/CMakeLists.txt b/be/src/transport/CMakeLists.txt
index bc1a40c..8a9eda8 100644
--- a/be/src/transport/CMakeLists.txt
+++ b/be/src/transport/CMakeLists.txt
@@ -30,4 +30,4 @@ add_library(ThriftSaslTransport
TSaslTransport.cpp
undef.cpp
)
-add_dependencies(ThriftSaslTransport thrift-deps)
\ No newline at end of file
+add_dependencies(ThriftSaslTransport gen-deps)
\ No newline at end of file
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/udf/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/udf/CMakeLists.txt b/be/src/udf/CMakeLists.txt
index 46100e5..0470716 100644
--- a/be/src/udf/CMakeLists.txt
+++ b/be/src/udf/CMakeLists.txt
@@ -26,10 +26,10 @@ set(EXECUTABLE_OUTPUT_PATH "${BUILD_OUTPUT_ROOT_DIRECTORY}/udf")
# can have dependencies on our other libs. The second version is shipped as part
# of the UDF sdk, which can't use other libs.
add_library(Udf udf.cc udf-ir.cc udf-test-harness.cc)
-add_dependencies(Udf thrift-deps)
+add_dependencies(Udf gen-deps)
add_library(ImpalaUdf udf.cc udf-ir.cc udf-test-harness.cc)
-add_dependencies(ImpalaUdf thrift-deps)
+add_dependencies(ImpalaUdf gen-deps)
set_target_properties(ImpalaUdf PROPERTIES COMPILE_FLAGS "-DIMPALA_UDF_SDK_BUILD")
ADD_UDF_TEST(udf-test)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/udf_samples/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/udf_samples/CMakeLists.txt b/be/src/udf_samples/CMakeLists.txt
index f9a34fd..b07b9d7 100644
--- a/be/src/udf_samples/CMakeLists.txt
+++ b/be/src/udf_samples/CMakeLists.txt
@@ -37,15 +37,15 @@ endfunction(COMPILE_TO_IR)
# Build the UDA/UDFs into a shared library.
add_library(udfsample SHARED udf-sample.cc)
-add_dependencies(udfsample thrift-deps)
+add_dependencies(udfsample gen-deps)
add_library(udasample SHARED uda-sample.cc hyperloglog-uda.cc)
-add_dependencies(udasample thrift-deps)
+add_dependencies(udasample gen-deps)
# Custom targest to cross compile UDA/UDF to ir
COMPILE_TO_IR(udf-sample.cc )
-add_dependencies(udf-sample-ir thrift-deps)
+add_dependencies(udf-sample-ir gen-deps)
COMPILE_TO_IR(uda-sample.cc )
-add_dependencies(uda-sample-ir thrift-deps)
+add_dependencies(uda-sample-ir gen-deps)
# This is an example of how to use the test harness to help develop UDF and UDAs.
add_executable(udf-sample-test udf-sample-test.cc)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/be/src/util/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/util/CMakeLists.txt b/be/src/util/CMakeLists.txt
index 3d92da7..3f18094 100644
--- a/be/src/util/CMakeLists.txt
+++ b/be/src/util/CMakeLists.txt
@@ -84,7 +84,7 @@ add_library(Util
webserver.cc
${MUSTACHE_SRC_DIR}/mustache.cc
)
-add_dependencies(Util thrift-deps gen_ir_descriptions)
+add_dependencies(Util gen-deps gen_ir_descriptions)
# Squeasel requires C99 compatibility to build.
SET_SOURCE_FILES_PROPERTIES(${SQUEASEL_SRC_DIR}/squeasel.c
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/35f5c7bd/ext-data-source/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/ext-data-source/CMakeLists.txt b/ext-data-source/CMakeLists.txt
index 1a53278..d318493 100644
--- a/ext-data-source/CMakeLists.txt
+++ b/ext-data-source/CMakeLists.txt
@@ -15,6 +15,6 @@
# specific language governing permissions and limitations
# under the License.
-add_custom_target(ext-data-source ALL DEPENDS thrift-deps
+add_custom_target(ext-data-source ALL DEPENDS gen-deps
COMMAND $ENV{IMPALA_HOME}/bin/mvn-quiet.sh install -DskipTests
)
[4/4] incubator-impala git commit: KUDU-2041: Fix negotiation deadlock
Posted by ta...@apache.org.
KUDU-2041: Fix negotiation deadlock
With N threads in the negotiation threadpool, N or more concurrent
client negotiation attempts could starve any incoming server negotiation
tasks which used the same threadpool.
If the set of negotiation attempts forms a graph with a N cycles, the
negotiation could deadlock (at least until the negotiation timeout
expires) as all nodes in the system wait for a server request to
complete, but all nodes have dedicated all their resources to client
requests.
Fix: split the server and client tasks into two separate pools.
Testing: add a unit test which reproduces the issue, and passes with the
fix applied.
Change-Id: I38379eeaf7516d432708c2a2a285839f96c86d4f
Reviewed-on: http://gerrit.cloudera.org:8080/7177
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
Reviewed-on: http://gerrit.cloudera.org:8080/7742
Reviewed-by: Henry Robinson <he...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/cc4816b3
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/cc4816b3
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/cc4816b3
Branch: refs/heads/master
Commit: cc4816b3d6ae2ca00ce61b1bc442161bfbe6de3f
Parents: d5670d6
Author: Henry Robinson <he...@cloudera.com>
Authored: Fri Apr 14 15:54:11 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Wed Aug 30 04:04:08 2017 +0000
----------------------------------------------------------------------
be/src/kudu/rpc/messenger.cc | 24 +++++++++++++++++++-----
be/src/kudu/rpc/messenger.h | 8 ++++++--
be/src/kudu/rpc/reactor.cc | 4 +++-
be/src/kudu/rpc/rpc-test-base.h | 15 +++++++++++++--
be/src/kudu/rpc/rpc-test.cc | 24 ++++++++++++++++++++++++
5 files changed, 65 insertions(+), 10 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/cc4816b3/be/src/kudu/rpc/messenger.cc
----------------------------------------------------------------------
diff --git a/be/src/kudu/rpc/messenger.cc b/be/src/kudu/rpc/messenger.cc
index 1cec5bf..2884fba 100644
--- a/be/src/kudu/rpc/messenger.cc
+++ b/be/src/kudu/rpc/messenger.cc
@@ -333,7 +333,8 @@ void Messenger::Shutdown() {
// Need to shut down negotiation pool before the reactors, since the
// reactors close the Connection sockets, and may race against the negotiation
// threads' blocking reads & writes.
- negotiation_pool_->Shutdown();
+ client_negotiation_pool_->Shutdown();
+ server_negotiation_pool_->Shutdown();
for (Reactor* reactor : reactors_) {
reactor->Shutdown();
@@ -435,10 +436,14 @@ Messenger::Messenger(const MessengerBuilder &bld)
for (int i = 0; i < bld.num_reactors_; i++) {
reactors_.push_back(new Reactor(retain_self_, i, bld));
}
- CHECK_OK(ThreadPoolBuilder("negotiator")
- .set_min_threads(bld.min_negotiation_threads_)
- .set_max_threads(bld.max_negotiation_threads_)
- .Build(&negotiation_pool_));
+ CHECK_OK(ThreadPoolBuilder("client-negotiator")
+ .set_min_threads(bld.min_negotiation_threads_)
+ .set_max_threads(bld.max_negotiation_threads_)
+ .Build(&client_negotiation_pool_));
+ CHECK_OK(ThreadPoolBuilder("server-negotiator")
+ .set_min_threads(bld.min_negotiation_threads_)
+ .set_max_threads(bld.max_negotiation_threads_)
+ .Build(&server_negotiation_pool_));
}
Messenger::~Messenger() {
@@ -503,5 +508,14 @@ const scoped_refptr<RpcService> Messenger::rpc_service(const string& service_nam
}
}
+ThreadPool* Messenger::negotiation_pool(Connection::Direction dir) {
+ switch (dir) {
+ case Connection::CLIENT: return client_negotiation_pool_.get();
+ case Connection::SERVER: return server_negotiation_pool_.get();
+ }
+ DCHECK(false) << "Unknown Connection::Direction value: " << dir;
+ return nullptr;
+}
+
} // namespace rpc
} // namespace kudu
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/cc4816b3/be/src/kudu/rpc/messenger.h
----------------------------------------------------------------------
diff --git a/be/src/kudu/rpc/messenger.h b/be/src/kudu/rpc/messenger.h
index 1ba76a7..9a8ebab 100644
--- a/be/src/kudu/rpc/messenger.h
+++ b/be/src/kudu/rpc/messenger.h
@@ -30,6 +30,7 @@
#include "kudu/gutil/gscoped_ptr.h"
#include "kudu/gutil/ref_counted.h"
+#include "kudu/rpc/connection.h"
#include "kudu/rpc/response_callback.h"
#include "kudu/security/token.pb.h"
#include "kudu/util/locks.h"
@@ -227,7 +228,7 @@ class Messenger {
RpcAuthentication authentication() const { return authentication_; }
RpcEncryption encryption() const { return encryption_; }
- ThreadPool* negotiation_pool() const { return negotiation_pool_.get(); }
+ ThreadPool* negotiation_pool(Connection::Direction dir);
RpczStore* rpcz_store() { return rpcz_store_.get(); }
@@ -287,7 +288,10 @@ class Messenger {
std::vector<Reactor*> reactors_;
- gscoped_ptr<ThreadPool> negotiation_pool_;
+ // Separate client and server negotiation pools to avoid possibility of distributed
+ // deadlock. See KUDU-2041.
+ gscoped_ptr<ThreadPool> client_negotiation_pool_;
+ gscoped_ptr<ThreadPool> server_negotiation_pool_;
std::unique_ptr<security::TlsContext> tls_context_;
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/cc4816b3/be/src/kudu/rpc/reactor.cc
----------------------------------------------------------------------
diff --git a/be/src/kudu/rpc/reactor.cc b/be/src/kudu/rpc/reactor.cc
index e235dd4..df4a661 100644
--- a/be/src/kudu/rpc/reactor.cc
+++ b/be/src/kudu/rpc/reactor.cc
@@ -446,7 +446,9 @@ Status ReactorThread::StartConnectionNegotiation(const scoped_refptr<Connection>
TRACE("Submitting negotiation task for $0", conn->ToString());
auto authentication = reactor()->messenger()->authentication();
auto encryption = reactor()->messenger()->encryption();
- RETURN_NOT_OK(reactor()->messenger()->negotiation_pool()->SubmitClosure(
+ ThreadPool* negotiation_pool =
+ reactor()->messenger()->negotiation_pool(conn->direction());
+ RETURN_NOT_OK(negotiation_pool->SubmitClosure(
Bind(&Negotiation::RunNegotiation, conn, authentication, encryption, deadline)));
return Status::OK();
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/cc4816b3/be/src/kudu/rpc/rpc-test-base.h
----------------------------------------------------------------------
diff --git a/be/src/kudu/rpc/rpc-test-base.h b/be/src/kudu/rpc/rpc-test-base.h
index c40f546..c28218b 100644
--- a/be/src/kudu/rpc/rpc-test-base.h
+++ b/be/src/kudu/rpc/rpc-test-base.h
@@ -525,6 +525,11 @@ class RpcTestBase : public KuduTest {
DoStartTestServer<CalculatorService>(server_addr, enable_ssl);
}
+ void StartTestServerWithCustomMessenger(Sockaddr *server_addr,
+ const std::shared_ptr<Messenger>& messenger, bool enable_ssl = false) {
+ DoStartTestServer<GenericCalculatorService>(server_addr, enable_ssl, messenger);
+ }
+
// Start a simple socket listening on a local port, returning the address.
// This isn't an RPC server -- just a plain socket which can be helpful for testing.
Status StartFakeServer(Socket *listen_sock, Sockaddr *listen_addr) {
@@ -548,8 +553,14 @@ class RpcTestBase : public KuduTest {
}
template<class ServiceClass>
- void DoStartTestServer(Sockaddr *server_addr, bool enable_ssl = false) {
- server_messenger_ = CreateMessenger("TestServer", n_server_reactor_threads_, enable_ssl);
+ void DoStartTestServer(Sockaddr *server_addr, bool enable_ssl = false,
+ const std::shared_ptr<Messenger>& messenger = nullptr) {
+ if (!messenger) {
+ server_messenger_ =
+ CreateMessenger("TestServer", n_server_reactor_threads_, enable_ssl);
+ } else {
+ server_messenger_ = messenger;
+ }
std::shared_ptr<AcceptorPool> pool;
ASSERT_OK(server_messenger_->AddAcceptorPool(Sockaddr(), &pool));
ASSERT_OK(pool->Start(2));
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/cc4816b3/be/src/kudu/rpc/rpc-test.cc
----------------------------------------------------------------------
diff --git a/be/src/kudu/rpc/rpc-test.cc b/be/src/kudu/rpc/rpc-test.cc
index 47ca528..2378892 100644
--- a/be/src/kudu/rpc/rpc-test.cc
+++ b/be/src/kudu/rpc/rpc-test.cc
@@ -108,6 +108,30 @@ TEST_F(TestRpc, TestConnHeaderValidation) {
ASSERT_OK(serialization::ValidateConnHeader(Slice(buf, conn_hdr_len)));
}
+// Regression test for KUDU-2041
+TEST_P(TestRpc, TestNegotiationDeadlock) {
+ bool enable_ssl = GetParam();
+
+ // The deadlock would manifest in cases where the number of concurrent connection
+ // requests >= the number of threads. 1 thread and 1 cnxn to ourself is just the easiest
+ // way to reproduce the issue, because the server negotiation task must get queued after
+ // the client negotiation task if they share the same thread pool.
+ MessengerBuilder mb("TestRpc.TestNegotiationDeadlock");
+ mb.set_min_negotiation_threads(1)
+ .set_max_negotiation_threads(1)
+ .set_metric_entity(metric_entity_);
+ if (enable_ssl) mb.enable_inbound_tls();
+
+ shared_ptr<Messenger> messenger;
+ CHECK_OK(mb.Build(&messenger));
+
+ Sockaddr server_addr;
+ StartTestServerWithCustomMessenger(&server_addr, messenger, enable_ssl);
+
+ Proxy p(messenger, server_addr, GenericCalculatorService::static_service_name());
+ ASSERT_OK(DoTestSyncCall(p, GenericCalculatorService::kAddMethodName));
+}
+
// Test making successful RPC calls.
TEST_P(TestRpc, TestCall) {
// Set up server.