diff --git a/presto-docs/src/main/sphinx/presto_cpp/properties.rst b/presto-docs/src/main/sphinx/presto_cpp/properties.rst index f1a90b246e815..9a2dd8f911d51 100644 --- a/presto-docs/src/main/sphinx/presto_cpp/properties.rst +++ b/presto-docs/src/main/sphinx/presto_cpp/properties.rst @@ -129,9 +129,10 @@ The configuration properties of Presto C++ workers are described here, in alphab ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ * **Type:** ``integer`` -* **Default value:** ``number of hardware CPUs`` +* **Default value:** ``number of concurrent threads supported by the host`` - Number of drivers to use per task. Defaults to hardware CPUs. + Number of drivers to use per task. Defaults to the number of concurrent + threads supported by the host. ``query.max-memory-per-node`` ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/presto-native-execution/presto_cpp/main/common/Configs.cpp b/presto-native-execution/presto_cpp/main/common/Configs.cpp index 303a368e89eda..cdb666a1baeb4 100644 --- a/presto-native-execution/presto_cpp/main/common/Configs.cpp +++ b/presto-native-execution/presto_cpp/main/common/Configs.cpp @@ -37,6 +37,15 @@ std::string bool2String(bool value) { return value ? "true" : "false"; } +int getThreadCount() { + auto numThreads = std::thread::hardware_concurrency(); + // The spec says std::thread::hardware_concurrency() might return 0. + // But we depend on std::thread::hardware_concurrency() to create executors. + // Check to ensure numThreads is > 0. + VELOX_CHECK_GT(numThreads, 0); + return numThreads; +} + #define STR_PROP(_key_, _val_) \ { std::string(_key_), std::string(_val_) } #define NUM_PROP(_key_, _val_) \ @@ -76,21 +85,6 @@ std::string ConfigBase::capacityPropertyAsBytesString( velox::config::CapacityUnit::BYTE)); } -bool ConfigBase::registerProperty( - const std::string& propertyName, - const folly::Optional& defaultValue) { - if (registeredProps_.count(propertyName) != 0) { - PRESTO_STARTUP_LOG(WARNING) - << "Property '" << propertyName - << "' is already registered with default value '" - << registeredProps_[propertyName].value_or("") << "'."; - return false; - } - - registeredProps_[propertyName] = defaultValue; - return true; -} - folly::Optional ConfigBase::setValue( const std::string& propertyName, const std::string& value) { @@ -138,7 +132,7 @@ SystemConfig::SystemConfig() { BOOL_PROP(kHttpServerReusePort, false), BOOL_PROP(kHttpServerBindToNodeInternalAddressOnlyEnabled, false), NONE_PROP(kDiscoveryUri), - NUM_PROP(kMaxDriversPerTask, 16), + NUM_PROP(kMaxDriversPerTask, getThreadCount()), NONE_PROP(kTaskWriterCount), NONE_PROP(kTaskPartitionedWriterCount), NUM_PROP(kConcurrentLifespansPerTask, 1), @@ -292,14 +286,16 @@ std::string SystemConfig::prestoVersion() const { } std::string SystemConfig::poolType() const { - static const std::unordered_set kPoolTypes = {"LEAF", "INTERMEDIATE", "DEFAULT"}; - static constexpr std::string_view kPoolTypeDefault = "DEFAULT"; - auto value = optionalProperty(kPoolType).value_or(std::string(kPoolTypeDefault)); - VELOX_USER_CHECK( - kPoolTypes.count(value), - "{} must be one of 'LEAF', 'INTERMEDIATE', or 'DEFAULT'", - kPoolType); - return value; + static const std::unordered_set kPoolTypes = { + "LEAF", "INTERMEDIATE", "DEFAULT"}; + static constexpr std::string_view kPoolTypeDefault = "DEFAULT"; + auto value = optionalProperty(kPoolType).value_or( + std::string(kPoolTypeDefault)); + VELOX_USER_CHECK( + kPoolTypes.count(value), + "{} must be one of 'LEAF', 'INTERMEDIATE', or 'DEFAULT'", + kPoolType); + return value; } bool SystemConfig::mutableConfig() const { diff --git a/presto-native-execution/presto_cpp/main/common/Configs.h b/presto-native-execution/presto_cpp/main/common/Configs.h index 0a053a9409238..fd148e32a4794 100644 --- a/presto-native-execution/presto_cpp/main/common/Configs.h +++ b/presto-native-execution/presto_cpp/main/common/Configs.h @@ -42,13 +42,6 @@ class ConfigBase { config_ = std::move(config); } - /// Registers an extra property in the config. - /// Returns true if succeeded, false if failed (due to the property already - /// registered). - bool registerProperty( - const std::string& propertyName, - const folly::Optional& defaultValue = {}); - /// Adds or replaces value at the given key. Can be used by debugging or /// testing code. /// Returns previous value if there was any. diff --git a/presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp b/presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp index 39b83b585a688..c119480ae4654 100644 --- a/presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp +++ b/presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp @@ -11,8 +11,8 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include #include +#include #include #include "presto_cpp/main/common/ConfigReader.h" #include "presto_cpp/main/common/Configs.h" @@ -217,7 +217,7 @@ TEST_F(ConfigTest, optionalNodeConfigs) { TEST_F(ConfigTest, optionalSystemConfigsWithDefault) { SystemConfig config; init(config, {}); - ASSERT_EQ(config.maxDriversPerTask(), 16); + ASSERT_EQ(config.maxDriversPerTask(), std::thread::hardware_concurrency()); init(config, {{std::string(SystemConfig::kMaxDriversPerTask), "1024"}}); ASSERT_EQ(config.maxDriversPerTask(), 1024); } diff --git a/presto-native-execution/presto_cpp/main/tests/ServerOperationTest.cpp b/presto-native-execution/presto_cpp/main/tests/ServerOperationTest.cpp index a338882684ebf..e0c73446af4f0 100644 --- a/presto-native-execution/presto_cpp/main/tests/ServerOperationTest.cpp +++ b/presto-native-execution/presto_cpp/main/tests/ServerOperationTest.cpp @@ -240,7 +240,8 @@ TEST_F(ServerOperationTest, systemConfigEndpoint) { {.target = ServerOperation::Target::kSystemConfig, .action = ServerOperation::Action::kGetProperty}, &httpMessage); - EXPECT_EQ(getPropertyResponse, "16\n"); + EXPECT_EQ( + std::stoi(getPropertyResponse), std::thread::hardware_concurrency()); } } // namespace facebook::presto