Copilot commented on code in PR #16360:
URL: https://github.com/apache/pinot/pull/16360#discussion_r2221241853
##########
pinot-spi/src/main/java/org/apache/pinot/spi/trace/Tracing.java:
##########
@@ -323,25 +344,36 @@ public static void clear() {
public static void initializeThreadAccountant(PinotConfiguration config,
String instanceId,
InstanceType instanceType) {
- String factoryName =
config.getProperty(CommonConstants.Accounting.CONFIG_OF_FACTORY_NAME);
+ createThreadAccountant(config, instanceId, instanceType);
+ }
+
+ public static ThreadResourceUsageAccountant
createThreadAccountant(PinotConfiguration config, String instanceId,
+ InstanceType instanceType) {
_workloadBudgetManager = new WorkloadBudgetManager(config);
- if (factoryName == null) {
- LOGGER.warn("No thread accountant factory provided, using default
implementation");
- } else {
+ String factoryName =
config.getProperty(CommonConstants.Accounting.CONFIG_OF_FACTORY_NAME);
+ ThreadResourceUsageAccountant accountant = null;
+ if (factoryName != null) {
LOGGER.info("Config-specified accountant factory name {}",
factoryName);
try {
ThreadAccountantFactory threadAccountantFactory =
(ThreadAccountantFactory)
Class.forName(factoryName).getDeclaredConstructor().newInstance();
- boolean registered =
Tracing.register(threadAccountantFactory.init(config, instanceId,
instanceType));
LOGGER.info("Using accountant provided by {}", factoryName);
+ accountant = threadAccountantFactory.init(config, instanceId,
instanceType);
+ boolean registered = register(accountant);
if (!registered) {
- LOGGER.warn("ThreadAccountant {} register unsuccessful, as it is
already registered.", factoryName);
+ accountant = null;
+ LOGGER.warn("ThreadAccountant register unsuccessful, as it is
already registered.");
Review Comment:
Setting accountant to null when registration fails could lead to unexpected
behavior. Consider logging the existing accountant type or providing more
context about why registration failed.
```suggestion
LOGGER.warn("ThreadAccountant register unsuccessful for
accountant type {}, as it is already registered.",
accountant.getClass().getName());
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/trace/Tracing.java:
##########
@@ -138,7 +143,23 @@ private static Tracer createDefaultTracer() {
*/
private static DefaultThreadResourceUsageAccountant
createDefaultThreadAccountant() {
LOGGER.info("Using default thread accountant");
- return new DefaultThreadResourceUsageAccountant();
+ DefaultThreadResourceUsageAccountant accountant = new
DefaultThreadResourceUsageAccountant();
+ Holder._accountant = accountant;
+ ACCOUNTANT_REGISTRATION.set(accountant);
Review Comment:
Setting ACCOUNTANT_REGISTRATION after already setting Holder._accountant
creates potential race conditions. The registration should be set before
updating the holder to ensure consistency.
```suggestion
ACCOUNTANT_REGISTRATION.set(accountant);
Holder._accountant = accountant;
```
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/ResourceUsageAccountantFactory.java:
##########
@@ -315,7 +320,7 @@ class WatcherTask implements Runnable {
@Override
public void run() {
LOGGER.debug("Running timed task for {}", this.getClass().getName());
- while (true) {
+ while (!Thread.currentThread().isInterrupted()) {
Review Comment:
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.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -756,7 +765,7 @@ private void logQueryMonitorConfig() {
@Override
public void run() {
- while (true) {
+ while (!Thread.currentThread().isInterrupted()) {
Review Comment:
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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
