vvivekiyer commented on code in PR #16340:
URL: https://github.com/apache/pinot/pull/16340#discussion_r2305198135
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##########
@@ -672,9 +673,12 @@ public void start()
_serverConf.getProperty(Server.CONFIG_OF_ENABLE_THREAD_ALLOCATED_BYTES_MEASUREMENT,
Server.DEFAULT_THREAD_ALLOCATED_BYTES_MEASUREMENT));
// Initialize the thread accountant for query killing
+ PinotConfiguration threadAccountantConfigs =
_serverConf.subset(CommonConstants.PINOT_QUERY_SCHEDULER_PREFIX);
+ // This allows for custom implementations of WorkloadBudgetManager.
+ WorkloadBudgetManager workloadBudgetManager =
createWorkloadBudgetManager(threadAccountantConfigs);
_resourceUsageAccountant =
Tracing.ThreadAccountantOps.createThreadAccountant(
_serverConf.subset(CommonConstants.PINOT_QUERY_SCHEDULER_PREFIX),
_instanceId,
- org.apache.pinot.spi.config.instance.InstanceType.SERVER);
+ org.apache.pinot.spi.config.instance.InstanceType.SERVER,
workloadBudgetManager);
Review Comment:
Is this interface change needed? Why can't createAccountant() create an
instance of the appropriate BudgetManager based on configs?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/WorkloadBudgetManager.java:
##########
@@ -113,19 +113,27 @@ public void addOrUpdateWorkload(String workload, long
cpuBudgetNs, long memoryBu
memoryBudgetBytes);
}
+ /**
+ * Collects workload stats for CPU and memory usage.
Review Comment:
Can we create a WorkloadBudgetManager interface with this being the
DefaultWorkloadManager implementation?
The internal implementation can have logic for stats collection.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -530,6 +535,13 @@ public void start()
protected void registerExtraComponents(BrokerAdminApiApplication
brokerAdminApplication) {
}
+ /**
+ * Can be overridden to create a custom WorkloadBudgetManager.
+ */
+ protected WorkloadBudgetManager
createWorkloadBudgetManager(PinotConfiguration brokerConf) {
+ return new WorkloadBudgetManager(brokerConf);
Review Comment:
Do you want to create a factory here that returns the right BudgetManager
implementation?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/WorkloadBudgetManager.java:
##########
@@ -206,15 +218,19 @@ public boolean canAdmitQuery(String workload) {
BudgetStats stats = budget.getStats();
return stats._cpuRemaining > 0 && stats._memoryRemaining > 0;
}
-
/**
- * Represents remaining budget stats.
+ * Represents budget stats.
*/
public static class BudgetStats {
+ public final long _initialCpuBudget;
Review Comment:
(nit) Some comments need to be updated to reflect this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
