Allow Reset of ThreadResourceUsageAccountant in Tracing.java

[vrajat]

This PR breaks the shackles of allowing only one ThreadResourceUsageAccountant in a JVM due to the following code:

    static final ThreadResourceUsageAccountant ACCOUNTANT =
        ACCOUNTANT_REGISTRATION.get() == null ? createDefaultThreadAccountant() : ACCOUNTANT_REGISTRATION.get();

While this makes sense in production where the accountant is setup during startup, it made testing hard. Its not possible to change the accountant for every test. Consequently, ResourceManagerAccountantTest was not really testing the accountant created in that specific test.

Another issue was that it was not possible to get the accountant created by the broker or server and check its state in tests. This could only be done by carefully orchestrating the startup sequence. This made test triage hard for those unfamiliar with this module.

This also enables a class of tests where a new accountant can be re-initialized by restarting a server or broker. So a single integration test can check different combinations of the initialization code.

The final issue was that the watcher task ran in a static executor service. So even if a test created multiple thread accountants, only the first one's watcher task used to run. The others were queued.

The following are the main changes:
Holder.ACCOUNTANT is no more a static final variable. The final restriction has been removed. register, unregisterThreadAccountant, createDefaultAccountant and initializeThreadAccountant are available to setup the accountant correctly in Server, Broker and in tests. unregisterThreadAccountant is especially useful to reset the global state between tests.

createDefaultAccountant has been added to return the thread accountant that was created. A server or broker stores the accountant it created even if it wasnt registered with the global singleton.

The executor service is now a member variable of the accountant and not a static member of the class.

Closes #15231

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 44.61538% with 36 lines in your changes missing coverage. Please review.

Project coverage is 63.34%. Comparing base (1a476de) to head (58130ca).
Report is 489 commits behind head on master.

Files with missing lines	Patch %	Lines
...inot/controller/helix/ControllerRequestClient.java	0.00%	13 Missing ⚠️
.../pinot/server/starter/helix/BaseServerStarter.java	0.00%	9 Missing ⚠️
.../main/java/org/apache/pinot/spi/trace/Tracing.java	65.38%	5 Missing and 4 partials ⚠️
...re/accounting/PerQueryCPUMemAccountantFactory.java	66.66%	1 Missing and 1 partial ⚠️
...spi/utils/builder/ControllerRequestURLBuilder.java	0.00%	2 Missing ⚠️
...ore/accounting/ResourceUsageAccountantFactory.java	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #16360      +/-   ##
============================================
+ Coverage     62.90%   63.34%   +0.44%     
+ Complexity     1386     1363      -23     
============================================
  Files          2867     2984     +117     
  Lines        163354   173335    +9981     
  Branches      24952    26562    +1610     
============================================
+ Hits         102755   109803    +7048     
- Misses        52847    55143    +2296     
- Partials       7752     8389     +637     

Flag	Coverage Δ
custom-integration1	100.00% <ø> (ø)
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-11	63.31% <44.61%> (+0.44%)	⬆️
java-21	63.33% <44.61%> (+0.51%)	⬆️
skip-bytebuffers-false	?
skip-bytebuffers-true	?
temurin	63.34% <44.61%> (+0.44%)	⬆️
unittests	63.34% <44.61%> (+0.44%)	⬆️
unittests1	56.48% <65.85%> (+0.66%)	⬆️
unittests2	33.29% <3.07%> (-0.28%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[vrajat]

@praveenc7 cc

[Copilot]

Pull Request Overview

This PR modifies the ThreadResourceUsageAccountant system in Pinot to allow resetting and re-registering accountants, primarily to improve testability. The key changes remove the static final restriction on the accountant singleton and add proper lifecycle management methods.

• Removes static final restriction on ThreadResourceUsageAccountant singleton to allow reset/re-registration
• Adds lifecycle management methods (unregister, createThreadAccountant) for better test control
• Moves executor service from static to instance member to avoid task queuing issues in tests

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
Tracing.java	Core changes to make accountant non-final and add lifecycle methods
ThreadResourceUsageAccountant.java	Adds stopWatcherTask default method interface
ResourceUsageAccountantFactory.java	Moves executor to instance variable and implements stopWatcherTask
PerQueryCPUMemAccountantFactory.java	Moves executor to instance variable and implements stopWatcherTask
BaseServerStarter.java	Updates to use createThreadAccountant and store reference
BaseBrokerStarter.java	Updates to use createThreadAccountant and store reference
ResourceManagerAccountingTest.java	Updates tests to use new accountant lifecycle methods
OOMProtectionEnabledIntegrationTest.java	Adds integration tests for accountant reset functionality
ControllerRequestURLBuilder.java	Adds cluster config management URLs
ControllerRequestClient.java	Adds cluster config update/delete methods
ControllerTest.java	Adds cluster config test helper methods
CPUMemThreadLevelAccountingObjects.java	Clears error status when setting thread to idle

Comments suppressed due to low confidence (1)

pinot-spi/src/main/java/org/apache/pinot/spi/trace/Tracing.java:70

• [nitpick] The field name '_accountant' uses underscore prefix which is inconsistent with Java naming conventions for static fields. Consider renaming to 'ACCOUNTANT' to maintain consistency with other static final fields in the class.

    static ThreadResourceUsageAccountant _accountant =

[Copilot]

Setting ACCOUNTANT_REGISTRATION after already setting Holder._accountant creates potential race conditions. The registration should be set before updating the holder to ensure consistency.

Suggested change

	Holder._accountant = accountant;
	ACCOUNTANT_REGISTRATION.set(accountant);
	ACCOUNTANT_REGISTRATION.set(accountant);
	Holder._accountant = accountant;

[Copilot]

The while loop should also check the interrupted flag at the beginning of each iteration to ensure prompt response to interruption. Consider adding Thread.interrupted() check after the sleep/wait operations.

[Copilot]

The while loop should also check the interrupted flag at the beginning of each iteration to ensure prompt response to interruption. Consider adding Thread.interrupted() check after the sleep/wait operations.

[praveenc7]

@vrajat Is there a reason other than backward compatibility we want to still keep this interface around? I don't see it used anywhere other than some test. Maybe we can replace it with createThreadAccountant for those handful of cases, It might create confusion when to you what, wdyt?

[vrajat]

Agree. I think we should do a massive cleanup of all the deprecated functions as well. I haven't yet because it needs work across the workload budget manager as well. Let's coordinate next week ?