Repository: incubator-impala Updated Branches: refs/heads/master a59408b57 -> 265e39f89
IMPALA-3535: Ignore invalid per-pool default query options In 2.5 we added the ability to set per-pool default query options. A string of key-value pairs can be specified with a pool configuration. However, if any options fail to parse, then all the options are ignored. We want that behavior (and returning an error) when parsing the process-wide default query options on startup and when parsing the options sent from a client (e.g. in beeswax server) because an error can be returned immediately for the triggering action at that time (i.e. starting the impalad or submitting a query with the options set). This behavior is bad for the pool default query options because (a) the configuration is set by the administrator and there's nothing we can do until a query is submitted and (b) one invalid option shouldn't mean that other valid options aren't set. Change-Id: If04733b775963091b0314c65286df126fd812358 Reviewed-on: http://gerrit.cloudera.org:8080/3056 Reviewed-by: Dan Hecht <[email protected]> Tested-by: Internal 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/f067929f Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/f067929f Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/f067929f Branch: refs/heads/master Commit: f067929f3a2b66f60c9bc020df268dd13d1dbc02 Parents: a59408b Author: Matthew Jacobs <[email protected]> Authored: Thu May 12 17:34:42 2016 -0700 Committer: Tim Armstrong <[email protected]> Committed: Tue May 17 10:09:05 2016 -0700 ---------------------------------------------------------------------- be/src/service/impala-server.cc | 9 ++- be/src/service/query-options-test.cc | 27 +++++++++ be/src/service/query-options.cc | 13 ++-- be/src/service/query-options.h | 3 +- .../impala/util/TestRequestPoolService.java | 4 +- fe/src/test/resources/llama-site-test2.xml | 2 +- .../functional-query/queries/QueryTest/set.test | 4 +- tests/query_test/test_query_opts.py | 64 ++++++++++++++++++++ 8 files changed, 112 insertions(+), 14 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f067929f/be/src/service/impala-server.cc ---------------------------------------------------------------------- diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc index 25ef571..7ab3a81 100644 --- a/be/src/service/impala-server.cc +++ b/be/src/service/impala-server.cc @@ -735,9 +735,8 @@ void ImpalaServer::AddPoolQueryOptions(TQueryCtx* ctx, status = ParseQueryOptions(config.default_query_options, &pool_options, &set_pool_options_mask); if (!status.ok()) { - VLOG_RPC << "Not adding pool query options for query=" << ctx->query_id - << " ParseQueryOptions status: " << status.GetDetail(); - return; + VLOG_QUERY << "Ignoring errors while parsing default query options for pool=" + << resolved_pool << ", message: " << status.GetDetail(); } QueryOptionsMask overlay_mask = override_options_mask & set_pool_options_mask; @@ -1143,8 +1142,8 @@ void ImpalaServer::TransmitData( void ImpalaServer::InitializeConfigVariables() { QueryOptionsMask set_query_options; // unused - Status status = ParseQueryOptions(FLAGS_default_query_options, &default_query_options_, - &set_query_options); + Status status = ParseQueryOptions(FLAGS_default_query_options, + &default_query_options_, &set_query_options); if (!status.ok()) { // Log error and exit if the default query options are invalid. CLEAN_EXIT_WITH_ERROR(Substitute("Invalid default query options. Please check " http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f067929f/be/src/service/query-options-test.cc ---------------------------------------------------------------------- diff --git a/be/src/service/query-options-test.cc b/be/src/service/query-options-test.cc index 53767d9..f1d90c0 100644 --- a/be/src/service/query-options-test.cc +++ b/be/src/service/query-options-test.cc @@ -80,6 +80,33 @@ TEST(QueryOptions, SetFilterWait) { EXPECT_EQ(numeric_limits<int32_t>::max(), options.runtime_filter_wait_time_ms); } +TEST(QueryOptions, ParseQueryOptions) { + QueryOptionsMask expectedMask; + expectedMask.set(TImpalaQueryOptions::NUM_NODES); + expectedMask.set(TImpalaQueryOptions::MEM_LIMIT); + + { + TQueryOptions options; + QueryOptionsMask mask; + EXPECT_OK(ParseQueryOptions("num_nodes=1,mem_limit=42", &options, &mask)); + EXPECT_EQ(options.num_nodes, 1); + EXPECT_EQ(options.mem_limit, 42); + EXPECT_EQ(mask, expectedMask); + } + + { + TQueryOptions options; + QueryOptionsMask mask; + Status status = ParseQueryOptions("num_nodes=1,mem_limit:42,foo=bar,mem_limit=42", + &options, &mask); + EXPECT_EQ(options.num_nodes, 1); + EXPECT_EQ(options.mem_limit, 42); + EXPECT_EQ(mask, expectedMask); + EXPECT_FALSE(status.ok()); + EXPECT_EQ(status.msg().details().size(), 2); + } +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); MemInfo::Init(); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f067929f/be/src/service/query-options.cc ---------------------------------------------------------------------- diff --git a/be/src/service/query-options.cc b/be/src/service/query-options.cc index d12ce1f..83e3548 100644 --- a/be/src/service/query-options.cc +++ b/be/src/service/query-options.cc @@ -107,7 +107,7 @@ Status impala::SetQueryOption(const string& key, const string& value, TQueryOptions* query_options, QueryOptionsMask* set_query_options_mask) { int option = GetQueryOptionForKey(key); if (option < 0) { - return Status(Substitute("Ignoring invalid configuration option: $0", key)); + return Status(Substitute("Invalid query option: $0", key)); } else { switch (option) { case TImpalaQueryOptions::ABORT_ON_ERROR: @@ -421,17 +421,22 @@ Status impala::ParseQueryOptions(const string& options, TQueryOptions* query_opt if (options.length() == 0) return Status::OK(); vector<string> kv_pairs; split(kv_pairs, options, is_any_of(","), token_compress_on); + // Construct an error status which is used to aggregate errors encountered during + // parsing. It is only returned if the number of error details is greater than 0. + Status errorStatus = Status::Expected("Errors parsing query options"); for (string& kv_string: kv_pairs) { trim(kv_string); if (kv_string.length() == 0) continue; vector<string> key_value; split(key_value, kv_string, is_any_of("="), token_compress_on); if (key_value.size() != 2) { - return Status(Substitute("Ignoring invalid configuration option $0: bad format " - "(expected 'key=value')", kv_string)); + errorStatus.MergeStatus( + Status(Substitute("Invalid configuration option '$0'.", kv_string))); + continue; } - RETURN_IF_ERROR(SetQueryOption(key_value[0], key_value[1], query_options, + errorStatus.MergeStatus(SetQueryOption(key_value[0], key_value[1], query_options, set_query_options_mask)); } + if (errorStatus.msg().details().size() > 0) return errorStatus; return Status::OK(); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f067929f/be/src/service/query-options.h ---------------------------------------------------------------------- diff --git a/be/src/service/query-options.h b/be/src/service/query-options.h index 60c122d..c86bc6b 100644 --- a/be/src/service/query-options.h +++ b/be/src/service/query-options.h @@ -111,7 +111,8 @@ Status SetQueryOption(const std::string& key, const std::string& value, /// Parse a "," separated key=value pair of query options and set it in 'query_options'. /// If the same query option is specified more than once, the last one wins. The /// set_query_options_mask bitmask is updated to reflect the query options which were -/// set. Return an error if the input is invalid (bad format or invalid query option). +/// set. Returns an error status containing an error detail for any invalid options (e.g. +/// bad format or invalid query option), but all valid query options are still handled. Status ParseQueryOptions(const std::string& options, TQueryOptions* query_options, QueryOptionsMask* set_query_options_mask); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f067929f/fe/src/test/java/com/cloudera/impala/util/TestRequestPoolService.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/com/cloudera/impala/util/TestRequestPoolService.java b/fe/src/test/java/com/cloudera/impala/util/TestRequestPoolService.java index c40ca0f..8e54b88 100644 --- a/fe/src/test/java/com/cloudera/impala/util/TestRequestPoolService.java +++ b/fe/src/test/java/com/cloudera/impala/util/TestRequestPoolService.java @@ -250,8 +250,10 @@ public class TestRequestPoolService { // Test pool limit changes checkPoolConfigResult("root", 15, 100, -1, 30000L, ""); + // not_a_valid_option=foo.bar gets filtered out when parsing the query options on + // the backend, but it should be observed coming from the test file here. checkPoolConfigResult("root.queueA", 1, 30, 100000 * ByteUnits.MEGABYTE, - 50L, "mem_limit=128m,query_timeout_s=5"); + 50L, "mem_limit=128m,query_timeout_s=5,not_a_valid_option=foo.bar"); checkPoolConfigResult("root.queueB", 5, 10, -1, 60000L, ""); checkPoolConfigResult("root.queueC", 10, 30, 128 * ByteUnits.MEGABYTE, 30000L, "mem_limit=2048m,query_timeout_s=60"); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f067929f/fe/src/test/resources/llama-site-test2.xml ---------------------------------------------------------------------- diff --git a/fe/src/test/resources/llama-site-test2.xml b/fe/src/test/resources/llama-site-test2.xml index 8b1e3d1..2e01f2e 100644 --- a/fe/src/test/resources/llama-site-test2.xml +++ b/fe/src/test/resources/llama-site-test2.xml @@ -41,7 +41,7 @@ </property> <property> <name>impala.admission-control.pool-default-query-options.root.queueA</name> - <value>mem_limit=128m,query_timeout_s=5</value> + <value>mem_limit=128m,query_timeout_s=5,not_a_valid_option=foo.bar</value> </property> <property> <name>llama.am.throttling.maximum.placed.reservations.root.queueC</name> http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f067929f/testdata/workloads/functional-query/queries/QueryTest/set.test ---------------------------------------------------------------------- diff --git a/testdata/workloads/functional-query/queries/QueryTest/set.test b/testdata/workloads/functional-query/queries/QueryTest/set.test index 361fc9b..e405caf 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/set.test +++ b/testdata/workloads/functional-query/queries/QueryTest/set.test @@ -135,7 +135,7 @@ The PARQUET_FILE_SIZE query option must be less than 2GB. ---- QUERY set foo=bar ---- CATCH -Ignoring invalid configuration option: foo +Invalid query option: foo ==== ---- QUERY set parquet_compression_codec=bar @@ -255,4 +255,4 @@ explain select count(distinct double_col) from functional.alltypesagg; '03:EXCHANGE [HASH(double_col)]' '01:AGGREGATE' '00:SCAN HDFS [functional.alltypesagg]' -==== \ No newline at end of file +==== http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f067929f/tests/query_test/test_query_opts.py ---------------------------------------------------------------------- diff --git a/tests/query_test/test_query_opts.py b/tests/query_test/test_query_opts.py new file mode 100644 index 0000000..6b6cd24 --- /dev/null +++ b/tests/query_test/test_query_opts.py @@ -0,0 +1,64 @@ +# Copyright (c) 2016 Cloudera, Inc. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# Tests for exercising query options that can be set in various ways. +# TODO: Add custom cluster tests for process default_query_options, but we need +# to make it easier to handle startup failures (right now it waits 60sec to +# timeout). + +from ImpalaService import ImpalaHiveServer2Service +from TCLIService import TCLIService + +from tests.beeswax.impala_beeswax import ImpalaBeeswaxException +from tests.common.custom_cluster_test_suite import CustomClusterTestSuite +from tests.common.impala_test_suite import ImpalaTestSuite +from tests.common.test_dimensions import create_exec_option_dimension +from tests.hs2.hs2_test_suite import HS2TestSuite, needs_session, operation_id_to_query_id + +class TestQueryOptions(ImpalaTestSuite): + @classmethod + def get_workload(cls): + return 'functional-query' + + @classmethod + def add_test_dimensions(cls): + super(TestQueryOptions, cls).add_test_dimensions() + cls.TestMatrix.add_constraint(lambda v:\ + v.get_value('table_format').file_format == 'text') + cls.TestMatrix.add_dimension(create_exec_option_dimension( + cluster_sizes=[0], disable_codegen_options=[False], batch_sizes=[0])) + + def test_set_invalid_query_option(self, vector): + ex = self.execute_query_expect_failure(self.client, "select 1", {'foo':'bar'}) + assert "invalid query option: foo" in str(ex).lower() + +class TestQueryOptionsHS2(HS2TestSuite): + @classmethod + def add_test_dimensions(cls): + super(TestQueryOptions, cls).add_test_dimensions() + cls.TestMatrix.add_constraint(lambda v:\ + v.get_value('table_format').file_format == 'text') + cls.TestMatrix.add_dimension(create_exec_option_dimension( + cluster_sizes=[0], disable_codegen_options=[False], batch_sizes=[0])) + + @needs_session() + def test_set_invalid_query_option(self): + """Tests that GetOperationStatus returns a valid result for a running query""" + execute_statement_req = TCLIService.TExecuteStatementReq() + execute_statement_req.sessionHandle = self.session_handle + execute_statement_req.confOverlay = {"foo":"bar"} + execute_statement_req.statement = "select 1" + execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req) + TestQueryOptionsHS2.check_response(execute_statement_resp, + TCLIService.TStatusCode.ERROR_STATUS, "Invalid query option: foo")
