Copilot commented on code in PR #16040:
URL: https://github.com/apache/pinot/pull/16040#discussion_r2227650947
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -528,8 +530,8 @@ private BrokerResponse query(QueryEnvironment.CompiledQuery
query, long requestI
try {
String workloadName =
QueryOptionsUtils.getWorkloadName(query.getOptions());
- Tracing.ThreadAccountantOps.setupRunner(String.valueOf(requestId),
ThreadExecutionContext.TaskType.MSE,
- workloadName);
+ _resourceUsageAccountant.setupRunner(QueryThreadContext.getCid(),
CommonConstants.Accounting.ANCHOR_TASK_ID,
Review Comment:
The method call signature has changed from setupRunner(String, TaskType,
String) to setupRunner(String, String, TaskType, String). The addition of
CommonConstants.Accounting.ANCHOR_TASK_ID as a second parameter should be
verified to ensure this matches the expected API and the constant value is
appropriate for this usage.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java:
##########
@@ -265,7 +268,7 @@ private IndexedTable getIndexedTable(DataSchema dataSchema,
Collection<DataTable
futures[i] = reducerContext.getExecutorService().submit(new
TraceRunnable() {
@Override
public void runJob() {
- Tracing.ThreadAccountantOps.setupWorker(taskId, parentContext);
+ _resourceUsageAccountant.setupWorker(taskId,
ThreadExecutionContext.TaskType.SSE, parentContext);
Review Comment:
The method call uses a hardcoded TaskType.SSE but the original code used
Tracing.ThreadAccountantOps.setupWorker(taskId, parentContext) which has a
different signature. This change in API usage should be verified to ensure the
correct task type is being used for this context.
```suggestion
ThreadExecutionContext.TaskType taskType =
determineTaskType(_queryContext);
_resourceUsageAccountant.setupWorker(taskId, taskType,
parentContext);
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -323,7 +325,8 @@ protected BrokerResponse handleRequest(long requestId,
String query, SqlNodeAndO
//Start instrumentation context. This must not be moved further below
interspersed into the code.
String workloadName =
QueryOptionsUtils.getWorkloadName(sqlNodeAndOptions.getOptions());
- Tracing.ThreadAccountantOps.setupRunner(String.valueOf(requestId),
workloadName);
+ _resourceUsageAccountant.setupRunner(QueryThreadContext.getCid(),
CommonConstants.Accounting.ANCHOR_TASK_ID,
Review Comment:
Similar to the MultiStageBrokerRequestHandler, this method call signature
has changed to include CommonConstants.Accounting.ANCHOR_TASK_ID as a second
parameter. The consistency and correctness of this API change should be
verified across all usage sites.
--
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
