You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "rhodo (via GitHub)" <gi...@apache.org> on 2025/04/30 08:56:39 UTC
[PR] Set conservative default for gRPC query sever worker thread count [pinot]
rhodo opened a new pull request, #15676:
URL: https://github.com/apache/pinot/pull/15676
Previously, the gRPC query server defaulted to using `2 * num_cores` threads for query workers. This PR adjusts the default to a lower cap (min(8, 2 * num_cores)) to reduce memory usage during request spikes, especially on machines with a high number of cores.
--
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] Set conservative default for gRPC query sever worker thread count [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #15676:
URL: https://github.com/apache/pinot/pull/15676#discussion_r2068276988
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcQueryServer.java:
##########
@@ -74,6 +75,8 @@ public class GrpcQueryServer extends PinotQueryServerGrpc.PinotQueryServerImplBa
// hashCode changes and the map is resized, the SslContext of the old hashCode will be lost.
private static final Map<Integer, SslContext> SERVER_SSL_CONTEXTS_CACHE = new ConcurrentHashMap<>();
+ private static final int DEFAULT_GRPC_QUERY_WORKER_THREAD = min(8, ResourceManager.DEFAULT_QUERY_WORKER_THREADS);
Review Comment:
nit: prefer `Math.min()`, instead of static import.
--
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] Set conservative default for gRPC query sever worker thread count [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15676:
URL: https://github.com/apache/pinot/pull/15676#issuecomment-2841396037
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15676?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 55.79%. Comparing base [(`4abddfc`)](https://app.codecov.io/gh/apache/pinot/commit/4abddfc8ff843640658a1a9a401b5b6f4ea4f623?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`3a418ef`)](https://app.codecov.io/gh/apache/pinot/commit/3a418ef686e0eeca2bfa26b1ee575ad8fc1f75ef?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 4 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/15676?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...che/pinot/core/transport/grpc/GrpcQueryServer.java](https://app.codecov.io/gh/apache/pinot/pull/15676?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Ftransport%2Fgrpc%2FGrpcQueryServer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvZ3JwYy9HcnBjUXVlcnlTZXJ2ZXIuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15676?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 (4abddfc) and HEAD (3a418ef). Click for more details.
>
> <details><summary>HEAD has 67 uploads less than BASE</summary>
>
>| Flag | BASE (4abddfc) | HEAD (3a418ef) |
>|------|------|------|
>|java-21|10|1|
>|unittests|6|1|
>|unittests1|3|1|
>|skip-bytebuffers-true|4|0|
>|temurin|15|1|
>|skip-bytebuffers-false|8|1|
>|unittests2|3|0|
>|java-11|5|0|
>|integration|9|0|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15676 +/- ##
============================================
- Coverage 62.92% 55.79% -7.14%
+ Complexity 1387 682 -705
============================================
Files 2866 2242 -624
Lines 163328 121583 -41745
Branches 24948 19347 -5601
============================================
- Hits 102779 67833 -34946
+ Misses 52803 48181 -4622
+ Partials 7746 5569 -2177
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15676/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/15676/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/15676/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/15676/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/15676/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/15676/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/15676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.79% <0.00%> (-7.13%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.79% <0.00%> (-7.14%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.79% <0.00%> (-7.14%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.79% <0.00%> (-7.14%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.79% <0.00%> (-0.09%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15676/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/15676?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] Set conservative default for gRPC query sever worker thread count [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #15676:
URL: https://github.com/apache/pinot/pull/15676
--
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] Set conservative default for gRPC query sever worker thread count [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #15676:
URL: https://github.com/apache/pinot/pull/15676#discussion_r2068283430
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcQueryServer.java:
##########
@@ -74,6 +75,8 @@ public class GrpcQueryServer extends PinotQueryServerGrpc.PinotQueryServerImplBa
// hashCode changes and the map is resized, the SslContext of the old hashCode will be lost.
private static final Map<Integer, SslContext> SERVER_SSL_CONTEXTS_CACHE = new ConcurrentHashMap<>();
+ private static final int DEFAULT_GRPC_QUERY_WORKER_THREAD = min(8, ResourceManager.DEFAULT_QUERY_WORKER_THREADS);
Review Comment:
Math.max(1, Math.min(8, ResourceManager.DEFAULT_QUERY_WORKER_THREADS))
--
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] Set conservative default for gRPC query sever worker thread count [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #15676:
URL: https://github.com/apache/pinot/pull/15676#discussion_r2068283430
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcQueryServer.java:
##########
@@ -74,6 +75,8 @@ public class GrpcQueryServer extends PinotQueryServerGrpc.PinotQueryServerImplBa
// hashCode changes and the map is resized, the SslContext of the old hashCode will be lost.
private static final Map<Integer, SslContext> SERVER_SSL_CONTEXTS_CACHE = new ConcurrentHashMap<>();
+ private static final int DEFAULT_GRPC_QUERY_WORKER_THREAD = min(8, ResourceManager.DEFAULT_QUERY_WORKER_THREADS);
Review Comment:
For k8s env, users might give 0.2 cpu , which could cause the work thread to 0.
Suggest to add this
`Math.max(2, Math.min(8, ResourceManager.DEFAULT_QUERY_WORKER_THREADS))` to ensure the bottom line.
Feel free to decide the minimal threads you want to put 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] Set conservative default for gRPC query sever worker thread count [pinot]
Posted by "rhodo (via GitHub)" <gi...@apache.org>.
rhodo commented on code in PR #15676:
URL: https://github.com/apache/pinot/pull/15676#discussion_r2069122363
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcQueryServer.java:
##########
@@ -74,6 +75,8 @@ public class GrpcQueryServer extends PinotQueryServerGrpc.PinotQueryServerImplBa
// hashCode changes and the map is resized, the SslContext of the old hashCode will be lost.
private static final Map<Integer, SslContext> SERVER_SSL_CONTEXTS_CACHE = new ConcurrentHashMap<>();
+ private static final int DEFAULT_GRPC_QUERY_WORKER_THREAD = min(8, ResourceManager.DEFAULT_QUERY_WORKER_THREADS);
Review Comment:
Thanks for the suggestion! Addressed comment
--
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