Tracing::ThreadUsageResourceAccountant cannot be isolated and tested in integration tests

[vrajat]

In integration tests, brokers and servers run in the same JVM. All of them share the same ThreadUsageResourceAccountant in a singleton in Tracing.java. This makes certain kinds of tests hard to write. For example:

• Throw a separate exception when a query is terminated because of OOM protection #14999
• Tests that involve relevant operations in a specific server.

Some tests use InterruptingAccountant. This class is useful to inject errors in the broker. However, it is not possible to inject errors in servers as the broker will error out first.

Couple of options are:

• Run a server in a different JVM. This is a major change in how tests have to be written. It will also make debug mode harder.
• Use dependency injection rather than a singleton to pass around an accountant object.

[vrajat]

cc @jasperjiaguo @vvivekiyer

[vrajat]

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.