You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "J-HowHuang (via GitHub)" <gi...@apache.org> on 2026/04/01 19:05:37 UTC
[PR] Allow semaphore with zero permit [pinot]
J-HowHuang opened a new pull request, #18074:
URL: https://github.com/apache/pinot/pull/18074
## Description
Other than MSE query throttler, this `AdjustableSemaphore` may be used in a context where 0 permits is acceptable (e.g. segment operation throttling, where we want to pause a certain operation)
## Change
Relax the argument check in `setPermits(int)` from `> 0` to `>= 0`
--
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] Allow semaphore with zero permit [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #18074:
URL: https://github.com/apache/pinot/pull/18074#discussion_r3024875664
##########
pinot-common/src/main/java/org/apache/pinot/common/concurrency/AdjustableSemaphore.java:
##########
@@ -44,7 +44,7 @@ public AdjustableSemaphore(int permits, boolean fair) {
* Synchronized to allow multiple threads to update permits concurrently
*/
public synchronized void setPermits(int permits) {
- Preconditions.checkArgument(permits > 0, "Permits must be a positive integer");
+ Preconditions.checkArgument(permits >= 0, "Permits must be a positive integer or zero");
Review Comment:
Yes. It will then be the caller of this semaphore to catch the value zero if that is unacceptable.
--
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] Allow semaphore with zero permit [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18074:
URL: https://github.com/apache/pinot/pull/18074#discussion_r3024111419
##########
pinot-common/src/main/java/org/apache/pinot/common/concurrency/AdjustableSemaphore.java:
##########
@@ -44,7 +44,7 @@ public AdjustableSemaphore(int permits, boolean fair) {
* Synchronized to allow multiple threads to update permits concurrently
*/
public synchronized void setPermits(int permits) {
- Preconditions.checkArgument(permits > 0, "Permits must be a positive integer");
+ Preconditions.checkArgument(permits >= 0, "Permits must be a positive integer or zero");
Review Comment:
```suggestion
Preconditions.checkArgument(permits >= 0, "Permits must be a non-negative integer");
```
##########
pinot-common/src/main/java/org/apache/pinot/common/concurrency/AdjustableSemaphore.java:
##########
@@ -44,7 +44,7 @@ public AdjustableSemaphore(int permits, boolean fair) {
* Synchronized to allow multiple threads to update permits concurrently
*/
public synchronized void setPermits(int permits) {
- Preconditions.checkArgument(permits > 0, "Permits must be a positive integer");
+ Preconditions.checkArgument(permits >= 0, "Permits must be a positive integer or zero");
Review Comment:
If the number of permits is set to 0, all threads trying to acquire this semaphore will block indefinitely - are you sure that's what you want to achieve here?
--
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] Allow semaphore with zero permit [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #18074:
URL: https://github.com/apache/pinot/pull/18074
--
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] Allow semaphore with zero permit [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18074:
URL: https://github.com/apache/pinot/pull/18074#issuecomment-4172675218
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18074?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 `50.00000%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 63.74%. Comparing base ([`abdb6cb`](https://app.codecov.io/gh/apache/pinot/commit/abdb6cba2d80ff07599194b000dabc95f519cbf6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6cac715`](https://app.codecov.io/gh/apache/pinot/commit/6cac7158a111ded6623e184ac4d95012499e3374?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 11 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18074?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../pinot/common/concurrency/AdjustableSemaphore.java](https://app.codecov.io/gh/apache/pinot/pull/18074?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fconcurrency%2FAdjustableSemaphore.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uY3VycmVuY3kvQWRqdXN0YWJsZVNlbWFwaG9yZS5qYXZh) | 0.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18074?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18074 +/- ##
============================================
+ Coverage 63.34% 63.74% +0.39%
+ Complexity 1543 1532 -11
============================================
Files 3200 3160 -40
Lines 194222 190784 -3438
Branches 29934 29282 -652
============================================
- Hits 123026 121606 -1420
+ Misses 61526 59636 -1890
+ Partials 9670 9542 -128
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18074/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/18074/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/18074/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/18074/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/18074/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/18074/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.22% <50.00%> (-7.09%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18074/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.71% <50.00%> (+0.39%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18074/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.74% <50.00%> (+0.39%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18074/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.74% <50.00%> (+0.40%)` | :arrow_up: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18074/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.25% <50.00%> (+0.73%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18074/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.13% <50.00%> (-0.13%)` | :arrow_down: |
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/18074?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