vrajat commented on code in PR #15798:
URL: https://github.com/apache/pinot/pull/15798#discussion_r2121359503
##########
pinot-spi/src/main/java/org/apache/pinot/spi/trace/Tracing.java:
##########
@@ -203,18 +204,20 @@ public void sampleUsageMSE() {
}
@Override
- public void updateQueryUsageConcurrently(String queryId) {
+ public void updateResourceUsageConcurrently(String identifier,
TrackingScope trackingScope) {
}
@Override
public final void createExecutionContext(@Nullable String queryId, int
taskId,
- ThreadExecutionContext.TaskType taskType, @Nullable
ThreadExecutionContext parentContext) {
+ ThreadExecutionContext.TaskType taskType, @Nullable
ThreadExecutionContext parentContext,
+ @Nullable String workloadName) {
Review Comment:
Can you added a overloaded function that does not accept `workloadName` ?
That will reduce the diff in files that are not affected by this change.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/trace/Tracing.java:
##########
@@ -267,14 +276,15 @@ public static class ThreadAccountantOps {
private ThreadAccountantOps() {
}
- public static void setupRunner(@Nonnull String queryId) {
- setupRunner(queryId, ThreadExecutionContext.TaskType.SSE);
+ public static void setupRunner(@Nonnull String queryId, @Nullable String
workloadName) {
Review Comment:
Same request here. If you add an overloaded fn. that does not accept
`workloadName`, that will reduce the diff.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/trace/Tracing.java:
##########
@@ -350,8 +362,8 @@ public static void sampleAndCheckInterruption() {
sample();
}
- public static void updateQueryUsageConcurrently(String queryId) {
- Tracing.getThreadAccountant().updateQueryUsageConcurrently(queryId);
+ public static void updateResourceUsageConcurrently(String resourceName,
TrackingScope trackingScope) {
Review Comment:
Is `TrackingScope` a feature specific to `WorkloadBudgetManager` ? Can it be
restricted to APIs for that class ?
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -59,6 +60,10 @@
* Accounting mechanism for thread task execution status and thread resource
usage sampling
* Design and algorithm see
*
https://docs.google.com/document/d/1Z9DYAfKznHQI9Wn8BjTWZYTcNRVGiPP0B8aEP3w_1jQ
+ *
+ * TODO: Functionalities in this Accountant are now supported in a more
generic ResourceUsageAccountantFactory. Keeping
Review Comment:
Based on the offline conversation, lets make sure we collaborate as we make
improvements to the framework.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -262,7 +268,11 @@ public int getEntryCount() {
}
@Override
- public void updateQueryUsageConcurrently(String queryId) {
+ public void updateResourceUsageConcurrently(String queryId, TrackingScope
resourceType) {
+ if (!resourceType.equals(TrackingScope.QUERY)) {
Review Comment:
Can you please help me understand scopes ? IIUC, scopes will be relevant to
the accountant added in this pR ? Why will that affect this implementation ?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -407,6 +408,9 @@ public void start()
timeSeriesRequestHandler, _responseStore);
_brokerRequestHandler.start();
+ // Initialize WorkloadBudgetManager for Query Workload Isolation.
Review Comment:
One more follow up question. Is the budget manager available only in the
broker ?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -407,6 +408,9 @@ public void start()
timeSeriesRequestHandler, _responseStore);
_brokerRequestHandler.start();
+ // Initialize WorkloadBudgetManager for Query Workload Isolation.
Review Comment:
As a follow up to our offline discussion, what is the effort to NOT make the
workload manager a global ? Can it be instantiated in the broker or server and
then propogated through dependency injection or function parameters ?
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
