Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add evb violation counter for exchange client and http server IO threadpool #24545

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+44 −1
Open

Add evb violation counter for exchange client and http server IO threadpool #24545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions presto-native-execution/presto_cpp/main/PrestoServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,12 @@ void PrestoServer::run() {
<< exchangeHttpIoExecutor_->getName() << "' has "
<< exchangeHttpIoExecutor_->numThreads()
<< " threads.";
for (auto evb : exchangeHttpIoExecutor_->getAllEventBases()) {
evb->setMaxLatency(
Copy link
Contributor

@aditi-pandit aditi-pandit Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kewang1024 : What is this for ? How do we turn this off ? Since we have a platform offering we would want to make this user (our client) configurable.

Are there any instructions for us on how to tune this for different clusters ?

@majetideepak @czentgr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is user-configurable, documentation should be included in this PR.

std::chrono::milliseconds(systemConfig->exchangeIoEvbViolationThresholdMs()),
[]() { RECORD_METRIC_VALUE(kCounterExchangeIoEvbViolation, 1); },
false);
}

const auto numExchangeHttpClientCpuThreads = std::max<size_t>(
systemConfig->exchangeHttpClientNumCpuThreadsHwMultiplier() *
Expand Down Expand Up @@ -524,6 +530,12 @@ void PrestoServer::run() {
PRESTO_STARTUP_LOG(INFO)
<< "HTTP Server CPU executor '" << httpSrvCpuExecutor_->getName()
<< "' has " << httpSrvCpuExecutor_->numThreads() << " threads.";
for (auto evb : httpSrvIoExecutor_->getAllEventBases()) {
evb->setMaxLatency(
std::chrono::milliseconds(systemConfig->httpSrvIoEvbViolationThresholdMs()),
[]() { RECORD_METRIC_VALUE(kCounterHttpServerIoEvbViolation, 1); },
false);
}
Comment on lines +533 to +538
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this setMaxLatency() for?
For executing http callback?
So what we doing here is setting a latency for our callbacks and if it is breached the given functor will be executed?
Do we suspect some of our callbacks talking long time on http executor?

Will this setMaxLatency() fail the http call if limit breached?

}
if (spillerExecutor_ != nullptr) {
PRESTO_STARTUP_LOG(INFO)
Expand Down
12 changes: 12 additions & 0 deletions presto-native-execution/presto_cpp/main/common/Configs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,8 @@ SystemConfig::SystemConfig() {
BOOL_PROP(kEnableRuntimeMetricsCollection, false),
BOOL_PROP(kPlanValidatorFailOnNestedLoopJoin, false),
STR_PROP(kPrestoDefaultNamespacePrefix, "presto.default"),
NUM_PROP(kExchangeIoEvbViolationThresholdMs, 1000),
NUM_PROP(kHttpSrvIoEvbViolationThresholdMs, 1000),
};
}

Expand Down Expand Up @@ -763,6 +765,16 @@ std::string SystemConfig::prestoDefaultNamespacePrefix() const {
return optionalProperty(kPrestoDefaultNamespacePrefix).value().append(".");
}

int32_t SystemConfig::exchangeIoEvbViolationThresholdMs() const {
return optionalProperty<int32_t>(kExchangeIoEvbViolationThresholdMs)
.value();
}

int32_t SystemConfig::httpSrvIoEvbViolationThresholdMs() const {
return optionalProperty<int32_t>(kHttpSrvIoEvbViolationThresholdMs)
.value();
}

NodeConfig::NodeConfig() {
registeredProps_ =
std::unordered_map<std::string, folly::Optional<std::string>>{
Expand Down
11 changes: 10 additions & 1 deletion presto-native-execution/presto_cpp/main/common/Configs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class ConfigBase {
protected:
ConfigBase()
: config_(std::make_unique<velox::config::ConfigBase>(
std::unordered_map<std::string, std::string>())){};
std::unordered_map<std::string, std::string>())) {};

// Check if all properties are registered.
void checkRegisteredProperties(
Expand Down Expand Up @@ -659,6 +659,11 @@ class SystemConfig : public ConfigBase {
static constexpr std::string_view kPrestoDefaultNamespacePrefix{
"presto.default-namespace"};

static constexpr std::string_view kExchangeIoEvbViolationThresholdMs{
"exchange.io-evb-violation-threshold-ms"};
static constexpr std::string_view kHttpSrvIoEvbViolationThresholdMs{
"http-server.io-evb-violation-threshold-ms"};

SystemConfig();

virtual ~SystemConfig() = default;
Expand Down Expand Up @@ -898,6 +903,10 @@ class SystemConfig : public ConfigBase {

bool prestoNativeSidecar() const;
std::string prestoDefaultNamespacePrefix() const;

int32_t exchangeIoEvbViolationThresholdMs() const;

int32_t httpSrvIoEvbViolationThresholdMs() const;
};

/// Provides access to node properties defined in node.properties file.
Expand Down
4 changes: 4 additions & 0 deletions presto-native-execution/presto_cpp/main/common/Counters.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,10 @@ void registerPrestoMetrics() {
99,
100);

DEFINE_METRIC(kCounterExchangeIoEvbViolation, facebook::velox::StatType::COUNT);

DEFINE_METRIC(kCounterHttpServerIoEvbViolation, facebook::velox::StatType::COUNT);

// NOTE: Metrics type exporting for thread pool executor counters are in
// PeriodicTaskManager because they have dynamic names and report configs. The
// following counters have their type exported there:
Expand Down
6 changes: 6 additions & 0 deletions presto-native-execution/presto_cpp/main/common/Counters.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,12 @@ constexpr std::string_view kCounterThreadPoolNumTotalTasksFormat{
constexpr std::string_view kCounterThreadPoolMaxIdleTimeNsFormat{
"presto_cpp.{}.max_idle_time_ns"};

/// ================== EVB Counters ====================
constexpr folly::StringPiece kCounterExchangeIoEvbViolation{
"presto_cpp.exchange_io_evb_violation_count"};
constexpr folly::StringPiece kCounterHttpServerIoEvbViolation{
"presto_cpp.http_server_io_evb_violation_count"};

/// ================== Memory Pushback Counters =================

/// Number of times memory pushback mechanism is triggered.
Expand Down
Loading