You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2025/07/31 23:14:59 UTC
[PR] Cleanup deprecated methods in ThreadResourceUsageAccountant [pinot]
Jackie-Jiang opened a new pull request, #16479:
URL: https://github.com/apache/pinot/pull/16479
- Cleanup the deprecated methods in `ThreadResourceUsageAccountant`
- Extract `MXBean` related methods into util class
- Move `WorkloadBudgetManager` to the correct package
- No functionality change
## Incompatible
Interface is changed in pinot-spi. This shouldn't cause backward compatible issue because the methods are not used, and the original PR deprecating the methods didn't really handle backward compatibility. This interface should be internal facing only right now
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Cleanup deprecated methods in ThreadResourceUsageAccountant [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #16479:
URL: https://github.com/apache/pinot/pull/16479
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Cleanup deprecated methods in ThreadResourceUsageAccountant [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16479:
URL: https://github.com/apache/pinot/pull/16479#issuecomment-3143311832
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16479?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `65.21739%` with `8 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.33%. Comparing base ([`62c7576`](https://app.codecov.io/gh/apache/pinot/commit/62c75763944aeacfce1bd8eef7ce9a2087c5d612?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`cda0570`](https://app.codecov.io/gh/apache/pinot/commit/cda05702ea13cf8dff8c1a46821ab11702421974?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16479?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...org/apache/pinot/common/utils/PinotAppConfigs.java](https://app.codecov.io/gh/apache/pinot/pull/16479?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Futils%2FPinotAppConfigs.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvUGlub3RBcHBDb25maWdzLmphdmE=) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ot/spi/accounting/ThreadResourceUsageProvider.java](https://app.codecov.io/gh/apache/pinot/pull/16479?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Faccounting%2FThreadResourceUsageProvider.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvYWNjb3VudGluZy9UaHJlYWRSZXNvdXJjZVVzYWdlUHJvdmlkZXIuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...t/common/restlet/resources/SystemResourceInfo.java](https://app.codecov.io/gh/apache/pinot/pull/16479?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Frestlet%2Fresources%2FSystemResourceInfo.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzdGxldC9yZXNvdXJjZXMvU3lzdGVtUmVzb3VyY2VJbmZvLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...counting/HeapUsagePublishingAccountantFactory.java](https://app.codecov.io/gh/apache/pinot/pull/16479?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Faccounting%2FHeapUsagePublishingAccountantFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hY2NvdW50aW5nL0hlYXBVc2FnZVB1Ymxpc2hpbmdBY2NvdW50YW50RmFjdG9yeS5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/pinot/spi/utils/ResourceUsageUtils.java](https://app.codecov.io/gh/apache/pinot/pull/16479?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Futils%2FResourceUsageUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvUmVzb3VyY2VVc2FnZVV0aWxzLmphdmE=) | 75.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (62c7576) and HEAD (cda0570). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (62c7576) | HEAD (cda0570) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|unittests2|2|0|
>|java-11|5|0|
>|integration|6|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16479 +/- ##
============================================
- Coverage 63.20% 56.33% -6.88%
+ Complexity 1363 699 -664
============================================
Files 3006 2353 -653
Lines 174110 129567 -44543
Branches 26640 20601 -6039
============================================
- Hits 110051 72995 -37056
+ Misses 55669 50547 -5122
+ Partials 8390 6025 -2365
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16479/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16479/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16479/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16479/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16479/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16479/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16479/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.33% <65.21%> (-6.85%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16479/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.33% <65.21%> (-6.88%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16479/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.33% <65.21%> (-6.87%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16479/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.33% <65.21%> (-0.02%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16479/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16479?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Cleanup deprecated methods in ThreadResourceUsageAccountant [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16479:
URL: https://github.com/apache/pinot/pull/16479#discussion_r2246553677
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -51,14 +51,12 @@ public class ThreadResourceUsageProvider {
private static boolean _isThreadCpuTimeMeasurementEnabled = false;
private static boolean _isThreadMemoryMeasurementEnabled = false;
- @Deprecated
- public long getThreadTimeNs() {
- return 0;
+ public static int getThreadCount() {
Review Comment:
The new methods getThreadCount() and getTotalStartedThreadCount() replace deprecated functionality but change from instance methods to static methods. This could break existing code that was calling these as instance methods, creating an incompatible API change.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/ResourceAggregator.java:
##########
@@ -21,39 +21,35 @@
import java.util.List;
-/**
- * Interface for aggregating CPU and memory usage of threads.
- */
+/// Interface for aggregating CPU and memory usage of threads.
public interface ResourceAggregator {
- /**
- * Update CPU usage for one-off cases where identifier is known before-hand. For example: broker inbound netty
- * thread where queryId and workloadName are already known.
- *
- * @param name identifier name - workload name, queryId, etc.
- * @param cpuTimeNs CPU time in nanoseconds
- */
- public void updateConcurrentCpuUsage(String name, long cpuTimeNs);
+ /// Updates CPU usage for one-off cases where identifier is known beforehand. For example: broker inbound netty thread
+ /// where queryId and workloadName are already known.
+ ///
+ /// @param name identifier name - queryId, workload name, etc.
+ /// @param cpuTimeNs CPU time in nanoseconds
+ void updateConcurrentCpuUsage(String name, long cpuTimeNs);
- /**
- * Update CPU usage for one-off cases where identifier is known before-hand. For example: broker inbound netty
- * @param name identifier name - workload name, queryId, etc.
- * @param memBytes memory usage in bytes
- */
- public void updateConcurrentMemUsage(String name, long memBytes);
+ /// Updates memory usage for one-off cases where identifier is known beforehand. For example: broker inbound netty
+ /// thread where queryId and workloadName are already known.
+ ///
+ /// @param name identifier name - queryId, workload name, etc.
+ /// @param memBytes memory usage in bytes
+ void updateConcurrentMemUsage(String name, long memBytes);
- // Cleanup of state after periodic aggregation is complete.
- public void cleanUpPostAggregation();
+ /// Cleans up state after periodic aggregation is complete.
+ void cleanUpPostAggregation();
- // Sleep time between aggregations.
- public int getAggregationSleepTimeMs();
+ /// Sleep time between aggregations.
+ int getAggregationSleepTimeMs();
- // Pre-aggregation step to be called before the aggregation of all thread entries.
- public void preAggregate(List<CPUMemThreadLevelAccountingObjects.ThreadEntry> anchorThreadEntries);
+ /// Pre-aggregation step to be called before the aggregation of all thread entries.
+ void preAggregate(List<CPUMemThreadLevelAccountingObjects.ThreadEntry> anchorThreadEntries);
- // Aggregation of each thread entry
- public void aggregate(Thread thread, CPUMemThreadLevelAccountingObjects.ThreadEntry threadEntry);
+ /// Aggregates on a thread entry.
+ void aggregate(Thread thread, CPUMemThreadLevelAccountingObjects.ThreadEntry threadEntry);
- // Post-aggregation step to be called after the aggregation of all thread entries.
- public void postAggregate();
+ /// Post-aggregation step to be called after the aggregation of all thread entries.
Review Comment:
The documentation style has been changed from standard Javadoc (/** ... */) to triple-slash comments (/// ...). This is inconsistent with Java documentation conventions and may not be properly recognized by documentation generation tools.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -559,8 +525,8 @@ public void reapFinishedTasks() {
Thread thread = entry.getKey();
if (!thread.isAlive()) {
+ LOGGER.debug("Thread: {} is no longer alive, removing it from _threadEntriesMap", thread.getName());
_threadEntriesMap.remove(thread);
Review Comment:
The debug log message was reordered to appear before the removal operation rather than after. While this change improves clarity, it represents a subtle behavioral change in logging that may affect debugging workflows.
```suggestion
_threadEntriesMap.remove(thread);
LOGGER.debug("Thread: {} is no longer alive, removed it from _threadEntriesMap", thread.getName());
```
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org