praveenc7 commented on code in PR #16340:
URL: https://github.com/apache/pinot/pull/16340#discussion_r2305566990
##########
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:
Since we currently have only one implementation, I avoided introducing a
factory prematurely. I was thinking a factory can be added later if we
introduce additional implementations.
As a reference, `PinotHelixResourceManager` also defines multiple custom
interfaces without relying on a factory. I borrowed a similar approach here to
keep things simpler until we actually need multiple implementations.
##########
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:
Same thoughts, on prematurely creating an interface when we don't additional
implementation yet.
I did explore this commit
https://github.com/apache/pinot/pull/16340/commits/6be32188dcce3e30af76f673c499d4c451ef4d65#diff-3c4497dbfe973a90e7a8cc7465bd99d71fc0e930bebe315c409e16036347754e
but felt it added additional code without the need for it now.
Given this can be expanded later when we have a additional implementation
##########
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:
The main reason is how we define custom implementations of
WorkloadBudgetManager. The trade-off here is between:
• Tweaking the interface a little to take in additional
parameter by passing in the WorkloadBudgetManager explicitly),
vs.
• Adding new configs (to define which WorkloadBudgetManager to
use) and internally handling this in createAccountant() .
Since we only have a single implementation today and don’t anticipate adding
another in the near future, introducing config toggles would add unnecessary
complexity and maintenance overhead. Passing the WorkloadBudgetManager directly
avoids this extra config surface.
--
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]
