vrajat commented on issue #15231:
URL: https://github.com/apache/pinot/issues/15231#issuecomment-2954641545
In PerQueryCpuMemAccountant, there is a
```
// the map to track stats entry for each thread, the entry will
automatically be added when one calls
// setThreadResourceUsageProvider on the thread, including but not
limited to
// server worker thread, runner thread, broker jetty thread, or broker
netty thread
private final ConcurrentHashMap<Thread,
CPUMemThreadLevelAccountingObjects.ThreadEntry> _threadEntriesMap
= new ConcurrentHashMap<>();
```
Entries are automatically added, when the following thread local is first
accessed:
```
private final
ThreadLocal<CPUMemThreadLevelAccountingObjects.ThreadEntry> _threadLocalEntry
= ThreadLocal.withInitial(() -> {
CPUMemThreadLevelAccountingObjects.ThreadEntry ret =
new CPUMemThreadLevelAccountingObjects.ThreadEntry();
_threadEntriesMap.put(Thread.currentThread(), ret);
LOGGER.info("Adding thread to _threadLocalEntry: {}",
Thread.currentThread().getName());
return ret;
}
);
```
To the best of my knowledge, the first access is always a call to
setupRunner or setupWorker. For example:
```
@Override
public void setupRunner(@Nullable String queryId, int taskId,
ThreadExecutionContext.TaskType taskType) {
_threadLocalEntry.get()._errorStatus.set(null);
if (queryId != null) {
_threadLocalEntry.get()
.setThreadTaskStatus(queryId,
CommonConstants.Accounting.ANCHOR_TASK_ID, taskType, Thread.currentThread());
}
}
```
These calls are made in specific places such as in OpChainSchedulerService
like
```
try (OpChain closeMe = operatorChain) {
Tracing.ThreadAccountantOps.setupWorker(operatorChain.getId().getStageId(),
ThreadExecutionContext.TaskType.MSE,
operatorChain.getParentContext());
LOGGER.trace("({}): Executing", operatorChain);
```
My opinion is that that there is no advantage to automatically add thread
entries to `_threadEntriesMap`. A developer still has to put the effort to
instrument the code with `setupRunner` and `setupWorker`. Instead, the code can
add an entry to a thread entries map in `OpChainSchedulerService` itself. This
is a concurrent map initialized in `BaseServerStarter` and injected during
construction. I dont see the point in effect having a global
`_threadEntriesMap`.
A similar argument can be made for the infra in the broker as well.
--
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
