This is an automated email from the ASF dual-hosted git repository. joemcdonnell pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 50f01352aa5b7a8b59cc46c90c8ba55c1e7f3e0a Author: jasonmfehr <[email protected]> AuthorDate: Wed Jun 25 17:19:36 2025 -0700 IMPALA-13235: Add Reusable GFlag Validators Adds three reusable GFlag validators in a header-only file. These validators can be passed directly to a GFlag DEFINE_validator() function. Sample usage: DEFINE_int32(myflag, 0, "Help text"); DEFINE_validator(myflag, ge_one) Workload management flags use a similar technique where a numeric validator is defined once and re-used throughout the flags. This validator has been moved to the new header-only file. Testing accomplished by the tests in gflag-validator-util-test.cc successfully passing locally and in a build. Change-Id: I752fd90ddfdad864d5d150a92495562d3d8ba0a2 Reviewed-on: http://gerrit.cloudera.org:8080/23098 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- be/src/util/CMakeLists.txt | 2 + be/src/util/gflag-validator-util-test.cc | 155 ++++++++++++++++++++++ be/src/util/gflag-validator-util.h | 55 ++++++++ be/src/workload_mgmt/workload-management-flags.cc | 27 ++-- 4 files changed, 221 insertions(+), 18 deletions(-) diff --git a/be/src/util/CMakeLists.txt b/be/src/util/CMakeLists.txt index e254620b4..26cb475cf 100644 --- a/be/src/util/CMakeLists.txt +++ b/be/src/util/CMakeLists.txt @@ -200,6 +200,7 @@ add_library(UtilTests STATIC error-util-test.cc filesystem-util-test.cc fixed-size-hash-table-test.cc + gflag-validator-util-test.cc hdfs-util-test.cc hdr-histogram-test.cc iceberg-utility-functions-test.cc @@ -260,6 +261,7 @@ ADD_UNIFIED_BE_LSAN_TEST(dict-test "DictTest.*") ADD_UNIFIED_BE_LSAN_TEST(error-util-test "ErrorMsg.*") ADD_UNIFIED_BE_LSAN_TEST(filesystem-util-test "FilesystemUtil.*") ADD_UNIFIED_BE_LSAN_TEST(fixed-size-hash-table-test "FixedSizeHash.*") +ADD_UNIFIED_BE_LSAN_TEST(gflag-validator-util-test "GFlagValidatorUtil.*") ADD_UNIFIED_BE_LSAN_TEST(hdfs-util-test HdfsUtilTest.*) ADD_UNIFIED_BE_LSAN_TEST(hdr-histogram-test HdrHistogramTest.*) ADD_UNIFIED_BE_LSAN_TEST(iceberg-utility-functions-test "IcebergPartitions.*") diff --git a/be/src/util/gflag-validator-util-test.cc b/be/src/util/gflag-validator-util-test.cc new file mode 100644 index 000000000..84032d8e7 --- /dev/null +++ b/be/src/util/gflag-validator-util-test.cc @@ -0,0 +1,155 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. + +#include "gflag-validator-util.h" + +#include <limits> + +#include "testutil/gtest-util.h" + +using namespace std; + +// +// Tests for the ge_zero validator utility function. +// +template<typename T> +static void run_ge_zero_success_int() { + EXPECT_TRUE(ge_zero<T>("flg", 0)); + EXPECT_TRUE(ge_zero<T>("flg", std::numeric_limits<T>::max())); +} + +TEST(GFlagValidatorUtil, TestGeZeroSuccessInt32) { + run_ge_zero_success_int<int32_t>(); +} + +TEST(GFlagValidatorUtil, TestGeZeroSuccessInt64) { + run_ge_zero_success_int<int64_t>(); +} + +template<typename T> +static void run_ge_zero_fail_int() { + EXPECT_FALSE(ge_zero<T>("flg", -1)); + EXPECT_FALSE(ge_zero<T>("flg", std::numeric_limits<T>::min())); +} +TEST(GFlagValidatorUtil, TestGeZeroFailInt32) { + run_ge_zero_fail_int<int32_t>(); +} + +TEST(GFlagValidatorUtil, TestGeZeroFailInt64) { + run_ge_zero_fail_int<int64_t>(); +} + +TEST(GFlagValidatorUtil, TestGeZeroSuccessDouble) { + EXPECT_TRUE(ge_zero<double_t>("flg", 0.0)); + EXPECT_TRUE(ge_zero<double_t>("flg", std::numeric_limits<double_t>::max())); +} + +TEST(GFlagValidatorUtil, TestGeZeroFailDouble) { + EXPECT_FALSE(ge_zero<double_t>("flg", -0.000000001)); + EXPECT_FALSE(ge_zero<double_t>("flg", -0.999999999)); + EXPECT_FALSE(ge_zero<double_t>("flg", -1.0)); + EXPECT_FALSE(ge_zero<double_t>("flg", std::numeric_limits<double_t>::lowest())); +} + +// +// Tests for the ge_one validator utility function. +// +template<typename T> +static void run_ge_one_success_int() { + EXPECT_TRUE(ge_one<T>("flg", 1)); + EXPECT_TRUE(ge_one<T>("flg", std::numeric_limits<T>::max())); +} + +TEST(GFlagValidatorUtil, TestGeOneSuccessInt32) { + run_ge_one_success_int<int32_t>(); +} + +TEST(GFlagValidatorUtil, TestGeOneSuccessInt64) { + run_ge_one_success_int<int64_t>(); +} + +template<typename T> +static void run_ge_one_fail_int() { + EXPECT_FALSE(ge_one<T>("flg", 0)); + EXPECT_FALSE(ge_one<T>("flg", -1)); + EXPECT_FALSE(ge_one<T>("flg", std::numeric_limits<T>::min())); +} + +TEST(GFlagValidatorUtil, TestGeOneFailInt32) { + run_ge_one_fail_int<int32_t>(); +} + +TEST(GFlagValidatorUtil, TestGeOneFailInt64) { + run_ge_one_fail_int<int64_t>(); +} + +TEST(GFlagValidatorUtil, TestGeOneSuccessDouble) { + EXPECT_TRUE(ge_one<double_t>("flg", 1.0)); + EXPECT_TRUE(ge_one<double_t>("flg", std::numeric_limits<double_t>::max())); +} + +TEST(GFlagValidatorUtil, TestGeOneFailDouble) { + EXPECT_FALSE(ge_one<double_t>("flg", 0.0)); + EXPECT_FALSE(ge_one<double_t>("flg", -1.0)); + EXPECT_FALSE(ge_one<double_t>("flg", std::numeric_limits<double_t>::lowest())); + EXPECT_FALSE(ge_one<double_t>("flg", 0.9999999999999999)); +} + +// +// Tests for the gt_zero validator utility function. +// +template<typename T> +static void run_gt_zero_success_int() { + EXPECT_TRUE(gt_zero<T>("flg", 1)); + EXPECT_TRUE(gt_zero<T>("flg", std::numeric_limits<T>::max())); +} + +TEST(GFlagValidatorUtil, TestGtZeroSuccessInt32) { + run_gt_zero_success_int<int32_t>(); +} + +TEST(GFlagValidatorUtil, TestGtZeroSuccessInt64) { + run_gt_zero_success_int<int64_t>(); +} + +template<typename T> +static void run_gt_zero_fail_int() { + EXPECT_FALSE(gt_zero<T>("flg", 0)); + EXPECT_FALSE(gt_zero<T>("flg", -1)); + EXPECT_FALSE(gt_zero<T>("flg", std::numeric_limits<T>::min())); +} + +TEST(GFlagValidatorUtil, TestGtZeroFailInt32) { + run_gt_zero_fail_int<int32_t>(); +} + +TEST(GFlagValidatorUtil, TestGtZeroFailInt64) { + run_gt_zero_fail_int<int64_t>(); +} + +TEST(GFlagValidatorUtil, TestGtZeroSuccessDouble) { + EXPECT_TRUE(gt_zero<double_t>("flg", 1.0)); + EXPECT_TRUE(gt_zero<double_t>("flg", 0.00000001)); + EXPECT_TRUE(gt_zero<double_t>("flg", 0.9999999999999999)); + EXPECT_TRUE(gt_zero<double_t>("flg", std::numeric_limits<double_t>::max())); +} + +TEST(GFlagValidatorUtil, TestGtZeroFailDouble) { + EXPECT_FALSE(gt_zero<double_t>("flg", 0.0)); + EXPECT_FALSE(gt_zero<double_t>("flg", -1.0)); + EXPECT_FALSE(gt_zero<double_t>("flg", std::numeric_limits<double_t>::lowest())); +} diff --git a/be/src/util/gflag-validator-util.h b/be/src/util/gflag-validator-util.h new file mode 100644 index 000000000..b43791e8d --- /dev/null +++ b/be/src/util/gflag-validator-util.h @@ -0,0 +1,55 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. + +#pragma once + +#include <glog/logging.h> + +// GFlag validator that asserts the provided value is greater than or equal to 0. +template<typename T> +bool ge_zero(const char* flagname, const T value) { + if (value >= 0){ + return true; + } + + LOG(ERROR) << "Flag '" << flagname << "' must be greater than or equal to 0."; + return false; +}; + +// GFlag validator that asserts the provided value is greater than or equal to 1. +// Double values greater than 0 but less than 1 will fail validation. +template<typename T> +bool ge_one(const char* flagname, const T value) { + if (value < 1) { + LOG(ERROR) << "Flag '" << flagname << "' must be greater than or equal to 1."; + return false; + } + + return true; +} + +// GFlag validator that asserts the provided value is greater than 0. +// Double values greater than 0 but less than 1 will pass validation. +template<typename T> +bool gt_zero(const char* flagname, const T value) { + if (value <= 0) { + LOG(ERROR) << "Flag '" << flagname << "' must be greater than 0."; + return false; + } + + return true; +} diff --git a/be/src/workload_mgmt/workload-management-flags.cc b/be/src/workload_mgmt/workload-management-flags.cc index bb65653c7..4632321f5 100644 --- a/be/src/workload_mgmt/workload-management-flags.cc +++ b/be/src/workload_mgmt/workload-management-flags.cc @@ -21,17 +21,12 @@ #include <gflags/gflags.h> #include <glog/logging.h> +#include "util/gflag-validator-util.h" + using namespace std; static regex alphanum_underscore_dash("^[A-Za-z0-9\\-_]+$"); -// Validator function asserting the value of a flag is greater than or equal to 0. -static const auto gt_eq_0 = [](const char* name, int32_t val) { - if (val >= 0) return true; - LOG(ERROR) << "Invalid value for --" << name << ": must be greater than or equal to 0"; - return false; -}; - DEFINE_bool(enable_workload_mgmt, false, "Specifies if Impala will automatically write completed queries in the query log " "table. If this value is set to true and then later removed, the query log table " @@ -86,11 +81,11 @@ DEFINE_int32_hidden(query_log_write_timeout_s, 0, "Specifies the query timeout i "seconds for inserts to the query log table. A value less than 1 indicates to use " "the same value as the query_log_write_interval_s flag. The value of this flag will " "be set in QUERY_TIMEOUT_S query option of the query log table insert DMLs."); -DEFINE_validator(query_log_write_timeout_s, gt_eq_0); +DEFINE_validator(query_log_write_timeout_s, ge_zero); DEFINE_int32(query_log_dml_exec_timeout_s, 120, "Value of the EXEC_TIME_LIMIT_S " "query option on the query log table insert dmls."); -DEFINE_validator(query_log_dml_exec_timeout_s, gt_eq_0); +DEFINE_validator(query_log_dml_exec_timeout_s, ge_zero); DEFINE_int32(query_log_max_queued, 3000, "Maximum number of records that can be queued " "before they are written to the impala query log table. This flag operates " @@ -99,7 +94,7 @@ DEFINE_int32(query_log_max_queued, 3000, "Maximum number of records that can be "matter how much time has passed since the last write. The countdown to the next " "write (based on the time period defined in the 'query_log_write_interval_s' flag) " "is not restarted."); -DEFINE_validator(query_log_max_queued, gt_eq_0); +DEFINE_validator(query_log_max_queued, ge_zero); DEFINE_string_hidden(workload_mgmt_maintenance_user, "impala", "Specifies the user that " "will be used to create and update the workload management database tables."); @@ -134,21 +129,21 @@ DEFINE_int32(query_log_max_sql_length, 16777216, "Maximum length of a sql statem "longer than this value is executed, the sql inserted into the completed queries " "table will be trimmed to this length. Any characters that need escaping will have " "their backslash character counted towards this limit."); -DEFINE_validator(query_log_max_sql_length, gt_eq_0); +DEFINE_validator(query_log_max_sql_length, ge_zero); DEFINE_int32(query_log_max_plan_length, 16777216, "Maximum length of the sql plan that " "will be recorded in the completed queries table. If a plan has a length longer than " "this value, the plan inserted into the completed queries table will be trimmed to " "this length. Any characters that need escaping will have their backslash character " "counted towards this limit."); -DEFINE_validator(query_log_max_plan_length, gt_eq_0); +DEFINE_validator(query_log_max_plan_length, ge_zero); DEFINE_int32_hidden(query_log_shutdown_timeout_s, 30, "Number of seconds to wait for " "the queue of completed queries to be drained to the query log table before timing " "out and continuing the shutdown process. The completed queries drain process runs " "after the shutdown process completes, thus the max shutdown time is extended by the " "value specified in this flag."); -DEFINE_validator(query_log_shutdown_timeout_s, gt_eq_0); +DEFINE_validator(query_log_shutdown_timeout_s, ge_zero); DEFINE_string(cluster_id, "", "Specifies an identifier string that uniquely represents " "this cluster. This identifier is included in the query log table if enabled."); @@ -164,11 +159,7 @@ DEFINE_validator(cluster_id, [](const char* name, const string& val) { DEFINE_int32_hidden(query_log_max_insert_attempts, 3, "Maximum number of times to " "attempt to insert a record into the completed queries table before abandining it."); -DEFINE_validator(query_log_max_insert_attempts, [](const char* name, int32_t val) { - if (val > 0) return true; - LOG(ERROR) << "Invalid value for --" << name << ": must be greater than 1"; - return false; -}); +DEFINE_validator(query_log_max_insert_attempts, ge_one); DEFINE_string(query_log_request_pool, "", "Specifies a pool or queue used by the queries " "that insert into the query log table. Empty value causes no pool to be set.");
